Various tweaks to redesigned structures #963

evanlinjin · 2023-05-03T05:04:23Z

Description

The following tweaks are made:

TxGraph::try_get_chain_position used to always exclude unconfirmed transactions with last_seen value of 0. However, what is the point of including a transaction in the graph if it cannot be part of the chain history? Additionally, maybe sometimes we don't wish to use the last_seen field at all. The new behavior will consider unconfirmed transactions with last_seen of 0.
Introduce LocalChain::insert_block. This is necessary as a LocalChain is updated with another LocalChain. The update LocalChain needs a simple way to insert blocks.
BlockId should not implement Anchor. Discussion: Introduce redesigned bdk_chain structures #926 (comment).
I have removed TxGraph::relevant_heights as it is ambiguous. A simpler and more flexible alternative method TxGraph::all_anchors is introduced.

Changelog notice

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

`TxGraph::try_get_chain_position` used to always exclude unconfirmed transactions with last_seen value of 0. However, what is the point of including a transaction in the graph if it cannot be part of the chain history? Additionally, maybe sometimes we don't wish to use the last_seen field at all. The new behavior will consider unconfirmed transactions with last_seen of 0.

LLFourn

I left some comments. Other misc things I noticed that are not changed in this PR but could be:

LocalChain::get_block(height) -> BlockId allows for invalid representation. It should be get_block_hash(height) -> BlockHash.
It would be nice if LocalChain had a way to doing .blocks and just getting a reference to the inner BTreeMap without using the AsRef implementation.
There are all these TODOs as doc comments for Local chain. Can you make issues out of the TODOs and remove them from comments.

crates/chain/src/local_chain.rs

crates/chain/src/tx_graph.rs

If `BlockId` implements `Anchor`, the meaning is ambiguous. We cannot tell whether it means the tx is anchors at the block, or whether it also means the tx is confirmed at that block. Instead, `ConfirmationHeightAnchor` and `ConfirmationTimeAnchor` structs are introduced as non-ambiguous `Anchor` implementations. Additionally, `TxGraph::relevant_heights` is removed because it is also ambiguous. What heights are deemed relevant? A simpler and more flexible method `TxGraph::all_anchors` is introduced instead.

* Introduce `LocalChain::inner` method to get the inner map of block height to hash. * Replace `LocalChain::get_block` (which outputted `BlockId`, thus able to return invalid representation) with `get_blockhash` that just returns a `BlockHash`. * Remove `TODO` comments that should be github tickets.

danielabrozzoni

ACK a56d289

A few comments but they don't have to be addressed

danielabrozzoni · 2023-05-05T09:07:08Z

crates/chain/src/indexed_tx_graph.rs

@@ -27,12 +28,14 @@ impl<A, I: Default> Default for IndexedTxGraph<A, I> {
    }
 }

-impl<A: Anchor, I: Indexer> IndexedTxGraph<A, I> {
+impl<A, I> IndexedTxGraph<A, I> {


I'm not sure how idiomatic this is, since we always end up using IndexedTxGraph with A: Anchor, I: Indexer, but I'm not opposed to this change if it's useful for us somehow

Even if it's not useful directly, it may allow building logic around/above it not require as many generic constraints. I would leave it as is unless if there are downsides to this?

crates/chain/tests/test_tx_graph.rs

crates/chain/tests/test_indexed_tx_graph.rs

evanlinjin · 2023-05-05T09:31:07Z

After a call with @LLFourn, the follow things will be added (either to this PR, or a separate PR):

The PersistBackend<T, C> trait currently has a generic T for the "in memory representation". This also constrains C to "accompany" T, thus requiring us to have something like Tracker in Implement persistence with the new structures #965. The "onioning" of these structures is unfortunate. However, there is a way to do PersistBackend without T and just rely on C! Let's experiment with that to see if it cleans up the API and reduces "onioning".
ChainOracle::get_chain_tip(&self) -> Result<Option<BlockId>, Self::Error> will be added. This is needed as we need something to input as the "static block" for the is_block_in_best_chain call. Additionally, a structure that is capable of implementing ChainOracle functionality should also have the functionality to return the tip of the best chain.
OwnedIndexer is bad. Here is something better:
```
pub OwnedTxOuts {
    SpkIndex;
    Iter: Iterator<Item = (Self::SpkIndex, OutPoint)>;
    fn owned_txouts(&self) -> Self::Iter;
}
```
This thing iterates over all outpoints that we own (alongside the spk index we give it). To get the owned UTXO set, we filter over this. Same thing for balance.

crates/chain/src/local_chain.rs

LLFourn

Left another idea. Approved. Happy to merge when @evanlinjin is happy.

Also, we can get rid of `LocalChain::get_blockhash`, since we can already expose the internal map. Additionally, tests and docs are improved.

evanlinjin marked this pull request as ready for review May 3, 2023 08:11

evanlinjin requested review from LLFourn, danielabrozzoni, rajarshimaitra and notmandatory May 3, 2023 08:19

evanlinjin self-assigned this May 3, 2023

evanlinjin added this to the 1.0.0-alpha.1 milestone May 3, 2023

notmandatory added the BDK 1.0.0 label May 4, 2023

LLFourn requested changes May 4, 2023

View reviewed changes

crates/chain/src/local_chain.rs Outdated Show resolved Hide resolved

crates/chain/src/tx_graph.rs Outdated Show resolved Hide resolved

notmandatory mentioned this pull request May 5, 2023

Test conflicting tx situations. #964

Closed

3 tasks

evanlinjin added 3 commits May 5, 2023 16:35

[chain_redesign] Add LocalChain::insert_block

085bf94

[chain_redesign] Relax generic constraints

4ae727a

evanlinjin force-pushed the chain_redesign_tweaks branch from 8de421f to 2ccc116 Compare May 5, 2023 08:39

danielabrozzoni approved these changes May 5, 2023

View reviewed changes

LLFourn reviewed May 5, 2023

View reviewed changes

crates/chain/src/local_chain.rs Outdated Show resolved Hide resolved

LLFourn reviewed May 5, 2023

View reviewed changes

crates/chain/src/local_chain.rs Outdated Show resolved Hide resolved

LLFourn reviewed May 5, 2023

View reviewed changes

crates/chain/src/local_chain.rs Show resolved Hide resolved

LLFourn approved these changes May 5, 2023

View reviewed changes

[chain_redesign] Rename LocalChain::inner() to blocks()

065c64a

Also, we can get rid of `LocalChain::get_blockhash`, since we can already expose the internal map. Additionally, tests and docs are improved.

evanlinjin merged commit e3c1370 into bitcoindevkit:master May 5, 2023

danielabrozzoni mentioned this pull request May 5, 2023

Resolve where to put Confirmation Height #951

Closed

notmandatory removed the BDK 1.0.0 label May 6, 2023

notmandatory mentioned this pull request Jul 4, 2023

Release 1.0.0-alpha.1 #1025

Closed

24 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Various tweaks to redesigned structures #963

Various tweaks to redesigned structures #963

evanlinjin commented May 3, 2023 •

edited

Loading

LLFourn left a comment

danielabrozzoni left a comment

danielabrozzoni May 5, 2023

evanlinjin May 5, 2023 •

edited

Loading

evanlinjin commented May 5, 2023 •

edited

Loading

LLFourn left a comment

Various tweaks to redesigned structures #963

Various tweaks to redesigned structures #963

Conversation

evanlinjin commented May 3, 2023 • edited Loading

Description

Changelog notice

Checklists

All Submissions:

New Features:

LLFourn left a comment

Choose a reason for hiding this comment

danielabrozzoni left a comment

Choose a reason for hiding this comment

danielabrozzoni May 5, 2023

Choose a reason for hiding this comment

evanlinjin May 5, 2023 • edited Loading

Choose a reason for hiding this comment

evanlinjin commented May 5, 2023 • edited Loading

LLFourn left a comment

Choose a reason for hiding this comment

evanlinjin commented May 3, 2023 •

edited

Loading

evanlinjin May 5, 2023 •

edited

Loading

evanlinjin commented May 5, 2023 •

edited

Loading