Skip to content
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

Performance issue with get_chain_position and associated methods for unconfirmed transactions #1665

Open
evanlinjin opened this issue Oct 29, 2024 · 3 comments · May be fixed by #1670
Open
Labels
audit Suggested as result of external code audit bug Something isn't working module-blockchain
Milestone

Comments

@evanlinjin
Copy link
Member

Describe the bug

get_chain_position for unconfirmed transactions is very inefficient if we have "nested conflicts". Since unconfirmed transactions that spend from evicted transactions are also evicted, we need to iterate over and check all unconfirmed descendants.

This problem is exacerbated in list_canonical_txs, filter_chain_txouts, etc. as these methods call get_chain_position for each transaction.

Worst case is O(n^2) for building the canonical tx history.

Proposed Solution

Let's pass an evicted_txs: &mut HashSet<Txid> input to get_chain_position which takes record of any tx that is deemed to be evicted from the canonical history. Note that descendants of evicted transactions are also evicted and should be included in evicted_txs. The logic to fill evicted_txs can stop when we insert an already-existing txid (since descendants of that txid will already be included in evicted_txs).

list_canonical_txs, filter_chain_txouts, etc. methods will construct an evicted_txs hashset which is passed to each internal call to try_get_chain_position.

@evanlinjin evanlinjin added the bug Something isn't working label Oct 29, 2024
@evanlinjin evanlinjin added this to the 1.0.0-beta milestone Oct 29, 2024
@evanlinjin evanlinjin added this to BDK Oct 29, 2024
@evanlinjin evanlinjin moved this to Todo in BDK Oct 29, 2024
@evanlinjin
Copy link
Member Author

@LagginTimes this one is for you.

@LLFourn
Copy link
Contributor

LLFourn commented Oct 29, 2024

ConceptNACK. I don't think we should change the API of get_chain_position. The problem is that the other batch methods should not be calling get_chain_position at all but rather implement a fast batch canonicalization algorithm.

@evanlinjin
Copy link
Member Author

@LLFourn and myself have discussed solutions for this in person and have come to consensus on the direction to move forwards with this.

In summary, we want to traverse backwards from transactions starting with latest last_seens, and fill up txid sets; must_be_canonical and must_not_be_canonical. An unconfirmed sub-graph with the highest last-seen is considered canonical if it does not conflict with confirmed transactions. Descendants of non-canonical txs are also non-canonical. Ancestors of canonical txs are automatically canonical.

@evanlinjin evanlinjin linked a pull request Nov 3, 2024 that will close this issue
8 tasks
@notmandatory notmandatory added module-blockchain audit Suggested as result of external code audit labels Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
audit Suggested as result of external code audit bug Something isn't working module-blockchain
Projects
Status: In Progress
Development

Successfully merging a pull request may close this issue.

3 participants