NethermindEth
if checkTxnPool true, try to fetch from pool first
Changes
- When fetching a tx, if
checkTxnPoolis set totrue, firstly try to fetch the tx from the pool
Types of changes
What types of changes does your code introduce?
- [ ] Bugfix (a non-breaking change that fixes an issue)
- [ ] New feature (a non-breaking change that adds functionality)
- [ ] Breaking change (a change that causes existing functionality not to work as expected)
- [x] Optimization
- [ ] Refactoring
- [ ] Documentation update
- [ ] Build-related changes
- [ ] Other: Description
Testing
Requires testing
- [ ] Yes
- [x] No
If yes, did you write tests?
- [ ] Yes
- [x] No
Documentation
Requires documentation update
- [ ] Yes
- [x] No
Requires explanation in Release Notes
- [ ] Yes
- [ ] No
Remarks
This heuristic should be better than the previous one, if the caller of the method expects the tx to be in the pool, we should check it first.
Why is that important? Theoretically we remove canonical transactions from pool in background thread, so we could have canonical transaction with a receipt still in the pool.
I wouldn't say it's important, but it seemed to make more sense to check the pool first, as it would bring small perf boost in scenarios where e.g. user subscribes to the pending transactions and then fetches each of these pending txs by hash (all of them would be in the pool). Given there is the edgewise you mentioned, feel free to close the PR if you think the scenario above isn't worth optimizng for :)
@marcindsobczak WDYT?
If we want we could potentially wait till channel becomes empty. We can add manual counter and reset event.
@marcindsobczak WDYT?
Made an attempt in #8348
IMO we can go with it as it is. It is used in two places:
eth_getRawTransactionByHashandeth_getTransactionByHash. For the first one, there is no issue, as it doesn't require the receipt. For the second, after the changes, we might occasionally return a recently processed transaction without the receipt. However, in my opinion, this is fine, and this method doesn’t need to be 100% accurate. If someone needs the receipt, they can always request it again.
I disagree, if someone is notified about the head change, we can't give him inconsistent view of transaction
Right. So we need either both this PR and #8348, or neither. We should consider if such changes are justified just for a micro-optimization.
Right. So we need either both this PR and #8348, or neither. We should consider if such changes are justified just for a micro-optimization.
So for suggested use - builder wants to get transactions from mempol ASAP this is probably not that micro as can skip db call. If you take into account that we can have hundreds of concurrent calls for transactions. But it is a concreate scenario.
Right. So we need either both this PR and #8348, or neither. We should consider if such changes are justified just for a micro-optimization.
So for suggested use - builder wants to get transactions from mempol ASAP this is probably not that micro as can skip db call. If you take into account that we can have hundreds of concurrent calls for transactions. But it is a concreate scenario.
Not only builder, but searcher as well, basically anyone who is subscribed to the mempool tx stream. IMO we should optimise for this, because, it's highly likely (in these scenarios) that TX will be in the pool, and in that case I think we should deliver it with the least possible latency. If the TX isn't in the pool then, overhead of firstly checking the pool doesn't matter.
