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

Introduce redesigned bdk_chain structures #926

Merged

Conversation

evanlinjin
Copy link
Member

@evanlinjin evanlinjin commented Mar 24, 2023

Description

This is part of #895

The initial bdk_chain structures allowed updating to be done without blocking IO (alongside many other benefits). However, the requirement to have transactions "perfectly positioned" in our SparseChain increased complexity of the syncing API. Updates needed to be meticulously crafted to properly connect with the original SparseChain. Additionally, it forced us to keep a local copy of the "best chain" data (which may not always be needed depending on the chain source).

The redesigned structures, as introduced by this PR, fixes these shortcomings.

  1. Instead of SparseChain, we introduce the ability to Anchor a transaction to multiple blocks that may or may not be in the same chain history. We expand TxGraph to records these anchors (while still maintaining the monotone nature of TxGraph). When updating our new TxGraph, we don't need to complicated are-we-connected checks that we need for SparseChain.
  2. The chain source, in combination with our newTxGraph is all that we need to determine the "chain position" of a transaction. The chain source only needs to implement a single trait ChainOracle. This typically does not need to be updated (as it is remote), although we can have a special ChainOracle implementation that is stored locally. This only needs block height and hash information, reducing the scope of non-monotine structures that need to be updated.

What is done:

  • TxGraph includes anchors (ids of blocks that the tx is seen in) and last-seem unix timestamp (for determining which non-confirmed tx we should consider as part of "best chain" if there are conflicts). This structure continues to be fully "monotone"; we can introduce data in any manner and not risk resulting in an inconsistent state.
  • LocalChain includes the "checkpoint" logic of SparseChain but removes the txid data. LocalChain implements the ChainOracle trait. Any blockchain-source can also implement the ChainOracle trait.
  • IndexedTxGraph is introduced and contains two fields; an internal TxGraph struct and a TxIndex implementation. These two fields will be updated atomically and can replace the functionality of keychain::Tracker.

What is in-progress:

What will be done external to this PR:

Changelog notice

  • Initial implementation of the bdk_chain redesign (as mentioned in Redesigning bdk_chain around a "ChainOracle" #895). Currently, only the bdk_chain core structures are implemented. Persistence and updates to the current examples and bdk::Wallet will be done in separate PRs.

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature

@evanlinjin evanlinjin force-pushed the chain_redesign_tx_graph_anchor branch from 7685ca2 to b9277fe Compare March 24, 2023 01:38
@evanlinjin evanlinjin changed the title Introduce TxAnchor trait Introduce BlockAnchor trait Mar 24, 2023
@evanlinjin evanlinjin force-pushed the chain_redesign_tx_graph_anchor branch 2 times, most recently from 2f2278b to 9d4f0ca Compare March 24, 2023 03:58
* Introduce `GraphedTx` struct to access transaction data of graphed
  transactions.
* Ability to insert/access anchors and "seen at" values for graphed
  transactions.
* `Additions` now records changes to anchors and last_seen_at.
@evanlinjin evanlinjin force-pushed the chain_redesign_tx_graph_anchor branch 4 times, most recently from aecc8f2 to 56bb46e Compare March 25, 2023 02:43
The chain oracle keeps track of the best chain, while the transaction
index indexes transaction data in relation to script pubkeys.

This commit also includes initial work on `IndexedTxGraph`.
@evanlinjin evanlinjin force-pushed the chain_redesign_tx_graph_anchor branch 4 times, most recently from da62bbb to a038c00 Compare March 27, 2023 03:11
Add methods to `TxGraph` and `IndexedTxGraph` that gets in-best-chain
data (such as transactions, txouts, unspent txouts).
@evanlinjin evanlinjin force-pushed the chain_redesign_tx_graph_anchor branch from a038c00 to 43b648f Compare March 27, 2023 04:44
Methods that list chain data have try and non-try versions. Both of
these versions now return an `Iterator`.

* Try versions return `Iterator<Item = Result>`.
* Non-try versions require the `ChainOracle` implementation to be
  `ChainOracle<Error = Infallible>`.
* Get mutable index from `IndexedChainGraph`.
* Also add `apply_additions` method to `TxIndex` trait.
…height

This is important as a `ChainOracle` implementation is updated
separately to an `IndexedTxGraph`.
@evanlinjin
Copy link
Member Author

@rajarshimaitra will be working on adding unit tests for TxGraph and then IndexedTxGraph.

@evanlinjin evanlinjin changed the title Introduce BlockAnchor trait Introduce IndexedTxGraph Mar 28, 2023
Copy link
Contributor

@rajarshimaitra rajarshimaitra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this looks good to me; two final nonblocking comments. The resolution for where to put the confirmation height should ideally be included in this PR. But if not can be done later. Tracking issue here #951

I am covering on the remaining tests for IndexedTxGraph. Need OwnedIndexer on SpkTxOutIndex for that. Will do it myself on my branch. If included in this PR anyway, would rebase on it.

crates/chain/src/spk_txout_index.rs Show resolved Hide resolved
crates/chain/tests/test_indexed_tx_graph.rs Show resolved Hide resolved
Copy link
Contributor

@LLFourn LLFourn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm approving this pending @evanlinjin addressing my comments in whatever way he wants. Thanks for all the work put into this @evanlinjin. It's sooooooo much better now. Looking forward to using this.

crates/chain/src/tx_graph.rs Outdated Show resolved Hide resolved
crates/chain/src/tx_graph.rs Outdated Show resolved Hide resolved
crates/chain/src/tx_graph.rs Outdated Show resolved Hide resolved
crates/chain/src/tx_graph.rs Show resolved Hide resolved
crates/chain/src/chain_data.rs Outdated Show resolved Hide resolved
crates/chain/src/tx_graph.rs Show resolved Hide resolved
evanlinjin and others added 4 commits April 28, 2023 18:54
* `Additions` now implements `Append` and uses `Append` to implement
  `append()`.
* `append()` logic enforces that `last_seen` values should only
  increase.
* Test written for `append()` with `last_seen` behaviour.
* `IndexedTxGraph::try_balance` should include "confirmed and spendable"
  into `confirmed` balance.
* `TxGraph::try_list_chain_unspents` filter logic should be reversed.
`SpkTxOutIndex` and `KeychainTxOutIndex` now both implement
`OwnedIndexer`.
These tests cover list_txout, list_utxo and balance methods.
@evanlinjin evanlinjin mentioned this pull request Apr 28, 2023
3 tasks
@rajarshimaitra
Copy link
Contributor

ACK b799a57

I had some clean-up pushes for the tests. Should I push them here? (I don't have push access in your branch).

Copy link
Contributor

@rajarshimaitra rajarshimaitra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK b799a57

I will push cleanups in a separate PR..

@evanlinjin evanlinjin changed the title Introduce IndexedTxGraph Introduce redesigned bdk_chain structures Apr 28, 2023
@evanlinjin evanlinjin added the Optech Make Me Famous! 🤩 For anything of interest to Optech newsletter label Apr 28, 2023
Copy link
Member

@danielabrozzoni danielabrozzoni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partial ACK b799a57 - good job! I haven't looked at the tests or at the methods implementations in depth, but I have reviewed the architecture and it looks good. I have a few non-blocking questions.

Let's goo 🚀

Comment on lines +38 to +43
/// Trait that "anchors" blockchain data to a specific block of height and hash.
///
/// I.e. If transaction A is anchored in block B, then if block B is in the best chain, we can
/// assume that transaction A is also confirmed in the best chain. This does not necessarily mean
/// that transaction A is confirmed in block B. It could also mean transaction A is confirmed in a
/// parent block of B.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at Anchor's definition, I'm wondering if there's something else it could make sense to implement it on, other than BlockId.
I'm not saying that Anchor shouldn't be a generic, I'm just wondering if you thought of any other structure that could be used as a anchor :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anchor implementations can be a lot more expressive. For example, Anchor with the exact confirmation height and/or time of the transaction. Or an Anchor with the exact merkle proof.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW I think anchor should not be implemented on BlockId. The semantics of it are ambiguous. Does it imply that the transaction is anchored to that block because the transaction is in the block or does that block merely imply that the transaction is in the block? I didn't mention this because I thought the discussion would come up later anyway.

}

/// A trait that extends [`Indexer`] to also index "owned" script pubkeys.
pub trait OwnedIndexer: Indexer {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(nit: this is not a super explicative name)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a side note, but I've actually started doubting whether we need this trait. It is basically used for filtering only (which can be done in layers above).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure but I think I mentioned that this trait has the wrong name in my mind. It should at least have Spk in the name because it can only answer queries about owning spks. It can just be called SpkOwner. There's also no need to require it implements Indexer. I would define it as:

pub trait TxoOwner {
    pub fn owns(&self, tx: &Transaction, vout: u32) -> bool;
}

because this is strictly more general and covers silent payments.

Comment on lines +17 to +19
pub struct LocalChain {
blocks: BTreeMap<u32, BlockHash>,
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the LocalChain sparse, or does it have all the blocks?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sparse!

@danielabrozzoni danielabrozzoni merged commit 139e3d3 into bitcoindevkit:master Apr 28, 2023
@rajarshimaitra rajarshimaitra mentioned this pull request Apr 29, 2023
3 tasks
@LLFourn LLFourn mentioned this pull request May 1, 2023
@notmandatory notmandatory mentioned this pull request Jul 4, 2023
24 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Optech Make Me Famous! 🤩 For anything of interest to Optech newsletter
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants