Further improve unconfirmed tx conflict resolution #1109
Conversation
LagginTimes
force-pushed
the
same_last_seen
branch
from
8fa2fd7 to
32da7e1
Compare
LagginTimes
force-pushed
the
same_last_seen
branch
3 times, most recently
from
5c65aca to
454d70d
Compare
LagginTimes
force-pushed
the
same_last_seen
branch
from
454d70d to
489c06c
Compare
There was a problem hiding this comment.
Having considered it again I'm not sure if it's a good idea to make this change. It's a half-fix. This tie-break will not necessarily reflect what full mempool nodes decide since it doesn't consider the feerates of parents etc. I do think it's a good idea to tie-break consistently and deterministically (so just arbitrarily choose txid by lexical order). We could do the simplest thing here and then make an issue to implement logic which reflects mempool rules of bitcoin core.
If we were to do this one I'd expect to have a few more tests. I don't think each branch here can be reach with each test.
cc @danielabrozzoni @evanlinjin wdyt?
|
I agree with Lloyd's points, tie-breaking by tx feerate doesn't make much sense, as it's the package feerate that counts. We can go with tie-breaking by txid for now. I also wonder how often it happens that two transactions have the same last_seen. |
LagginTimes
force-pushed
the
same_last_seen
branch
from
489c06c to
c4aea75
Compare
|
Updated to tiebreak by txid only. |
There was a problem hiding this comment.
ACK c4aea75
These two commits should be squashed.
Thanks @LagginTimes.
LagginTimes
force-pushed
the
same_last_seen
branch
from
c4aea75 to
bf2f7d7
Compare
|
As per today's discussion in the Lib Team Call, this PR probably needs to be rebased after the ci fix of #1182 |
LagginTimes
force-pushed
the
same_last_seen
branch
from
bf2f7d7 to
4861048
Compare
The tx conflict `Scenario` test for unconfirmed txs with the same last_seen has been amended for its corresponding conflict resolution bug fix.
LagginTimes
force-pushed
the
same_last_seen
branch
from
4861048 to
afbf83c
Compare
|
Rebased with #1182 CI fixes. |
| if conflicting_tx.last_seen_unconfirmed == *last_seen | ||
| && conflicting_tx.txid() > tx.txid() | ||
| { | ||
| // Conflicting tx has priority if txid of conflicting tx > txid of original tx |
There was a problem hiding this comment.
nit: this should be documented, but I don't want to hold up merging, so I'm going to do it as a part of #1188
| exp_chain_txouts: HashSet::from([("tx1", 0), ("tx_conflict_1", 0), ("tx_conflict_2", 0)]), | ||
| // correct output if filtered by fee rate: tx_conflict_1 | ||
| exp_unspents: HashSet::from([("tx_conflict_1", 0), ("tx_conflict_2", 0)]), | ||
| exp_chain_txs: HashSet::from(["tx1", "tx_conflict_2"]), |
There was a problem hiding this comment.
Same as above, this needs docs, will do it as a part of #1188
Description
Fixes #1102. If a conflicting tx has the same
last_seen, then we check lexicographical sorting of txids.Notes to the reviewers
The tests for this fix exist in the
TxTemplatestructure in #1064 which may need to be merged first.Checklists
All Submissions:
cargo fmtandcargo clippybefore committing