-
Notifications
You must be signed in to change notification settings - Fork 307
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Better tests for transaction conflict handling #1064
Better tests for transaction conflict handling #1064
Conversation
952930b
to
cd45f15
Compare
cd45f15
to
4d7c3fb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this, glad to see the progress. Just some comments!
d112807
to
83c92b6
Compare
ce6cb40
to
67d9a5e
Compare
67d9a5e
to
70d010c
Compare
70d010c
to
4e1d898
Compare
4e1d898
to
a86f76a
Compare
af19b5a
to
238f0f1
Compare
Please rebase on |
112e851
to
b00b406
Compare
In case you have never done that, https://git-scm.com/book/en/v2/Git-Tools-Rewriting-History can be of help :) |
7340a41
to
452627d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 452627d
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @LagginTimes. I've pointed out what I think is a bug that I think should be fixed here. The other things I've identified I leave to you to resolve them here or we can do it in follow up work.
The key things we should (eventually) resolve are:
- The worst case time complexity of the algorithm to determine whether a single tx is in the chain is roughly linear in the number of unconfirmed transactions. If we are determining whether a bunch of txouts are in the chain and every unconfirmed tx has one this means it's quadratic. I fear by peppering the mempool with a bunch of unconfirmed txs and RBFing them a lot you could denial of service attack systems using this algorithm.
- I think it'd be better if
TxDescendents
andTxAncestors
should be proximity to the root txid in the graph. The current order is depth first and at what "depth" the node is visited at depends on the order of the outputs or inputs that are being traversed. The iterator can give a depth of7
to a tx that has a direct relationship to the root tx.
As discussed with @evanlinjin, I'm picking this up as it's high priority :) |
Co-authored-by: Wei Chen <wzc110@gmail.com>
This commit also changes test_descendants_no_repeat to check the order of the transactions returned
Co-authored-by: Wei Chen <wzc110@gmail.com>
Co-authored-by: Wei Chen <wzc110@gmail.com>
452627d
to
df1ee4f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just pushed df1ee4f:
- Rewrite commit history
- Fix review comments
As a part of fixing the review comments I added a scenario in test_tx_graph_conflicts
, I just wanted to say @LagginTimes, the Scenario/TxTemplate
mechanism is super convenient! Good job :)
}, | ||
], | ||
// B and C should not appear in the list methods | ||
exp_chain_txs: HashSet::from(["A", "B'"]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't what we want to happen here is that B
and C
are canonical?
C
has the more recent last seen which should transitively mean that B
is chosen over B'
. In other words the effective last_seen
of a node is the max of all its descendants.
This PR at least makes the answer consistent but it seems we are actually asserting suboptimal behavior here. Can be done in a follow up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to give a bit of context, my reasoning behind this was: how would a node react if it saw B, then B', then C? And I think it would go like:
- B: fine, I'll keep this
- B': fine, I'll keep this, and drop B
- C: I'll drop this, as it's spending B, which I dropped already
Which I don't know if it's coherent with the rest of the txgraph and the last_seen logic (it probably isn't).
Anyways, I just pushed a fix, it should be fine now :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which I don't know if it's coherent with the rest of the txgraph and the last_seen logic (it probably isn't).
Indeed. TxGraph is montone so doesn't record the order of things entering into it. Our conflict resolution mechanism is last_seen
so I think we should take whichever subgraph has the tx with the latest one. Hopefully we can make this efficient!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved. Consider fixing https://github.com/bitcoindevkit/bdk/pull/1064/files#r1344900282
ACK df1ee4f
...try_get_chain_pos In try_get_chain_pos, when we notice that a transaction is not included in the best chain, we check the transactions in mempool to find conflicting ones, and decide based on that if our transaction is still in mempool or has been dropped. This commit adds a check for transactions conflicting with the unconfirmed ancestors of our tx. Co-authored-by: Wei Chen <wzc110@gmail.com>
Co-authored-by: Wei Chen <wzc110@gmail.com>
Co-authored-by: Wei Chen <wzc110@gmail.com>
Co-authored-by: Wei Chen <wzc110@gmail.com>
Co-authored-by: Wei Chen <wzc110@gmail.com>
df1ee4f
to
6d601a7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 6d601a7
Thanks @danielabrozzoni for taking this forward and @LagginTimes for the initial work.
afbf83c chain(fix): conflict resolution for txs with same last_seen (Wei Chen) Pull request description: ### 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 `TxTemplate` structure in #1064 which may need to be merged first. ### Checklists #### All Submissions: * [x] I've signed all my commits * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md) * [x] I ran `cargo fmt` and `cargo clippy` before committing ACKs for top commit: danielabrozzoni: ACK afbf83c Tree-SHA512: 91b8fbff305b715247501b861ab7ea9e9d9ef99248b05d14e01aacf7e64ad7826f35773e8998cf421dbd04f663714026084c6e817ac5365bce4844c8ea3b7e3f
Description
Fixes #1063.
This PR introduces a new
TxTemplate
struct to test different transaction conflict scenarios inTxGraph
.The following transaction conflict scenarios are tested:
These tests revealed that
TxGraph::walk_conflicts
was not checking ancestors of the root tx for conflicts.TxGraph::walk_conflicts
has been refactored to check for conflicting ancestor transactions by using a newTxAncestors
iterator inTxGraph
.Changelog notice
tx_template
moduleTxGraph::TxAncestors
iteratorTxGraph::walk_conflicts
to useTxGraph::TxAncestors
walk_ancestors
toTxGraph
Checklists
All Submissions:
New Features: