BlockId should not have a default and passing a chain tip should be an Option #1107 #1116

SpaghettiSats · 2023-09-11T09:32:49Z

Description

This pull request addresses the issue discussed in GitHub Issue #1107. The primary changes made in this PR are as follows:

BlockId Default Removal: Removed the default implementation for BlockId as it doesn't make sense for it to have a default value. Instead, we rely on Option<BlockId> to semantically represent a null block.
Chain Tip as Option: Changed the chain_tip parameter in various methods to accept an Option<BlockId> instead of using unwrap_or_default(). This aligns with Rust conventions for handling null values.
ChainOracle's get_chain_tip: While keeping get_chain_tip, it is now made more meaningful by adding functionality to fetch the chain tip from the chain argument when calling certain methods. This makes the get_chain_tip method more useful and justified.
API Improvement: Introduced a more convenient version of filter_chain_unspents that automatically fetches the chain tip from the chain argument, simplifying usage. The original method is retained as filter_subchain_unspents to maintain backward compatibility. Similar improvements are made for the following methods: get_chain_position, get_chain_spend, list_chain_txs, filter_chain_txouts, balance and of course also for their fallible versions.

Notes to the reviewers

This PR aims to improve the API by aligning it with Rust conventions for handling null values and making it more user-friendly.
Carefully reviewed the changes to ensure consistency with existing code and adherence to project guidelines.
Tests have been added for the new features and adjustments.

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

Bugfixes:

This pull request breaks the existing API.
I've added tests to reproduce the issue which are now passing (Not applicable in this case).
I'm linking the issue being fixed by this PR (GitHub Issue #1107).

realeinherjar

Do we really need to .clone() all the Into<BlockId>?
Can't we pass by reference in some of these?

realeinherjar · 2023-09-11T20:48:25Z

crates/chain/src/local_chain.rs

+        chain_tip: Option<B>,
    ) -> Result<Option<bool>, Self::Error> {
+        let chain_tip: BlockId = match chain_tip {
+            Some(x) => x.into(),
+            None => return Ok(None),
+        };
+


Wouldn't it better to return Result<bool, Self::Error>?
Result<Option<bool>, Self::Error> seems unnecessary

There are three possibilities:

Some(true): The block is in the chain

Some(false): The block is not in the chain

None we don't know if the block is in the chain.

Maybe in the future, we can consider turning this into an enum?

Not sure. Would just add documentation as far as I can see. Documenting the trait method seems clear enough. I could be wrong.

LLFourn

Thanks for doing this tedious work @SpaghettiSats. General comments:

No need to add specific tests for these. Try to avoid as much as possible changes to the test files in this PR. I think it's quite hard to implement these convenience methods incorrectly.
Docs need to explain what a subchain is. Liberally refer to the other method e.g. subchain_X is like [X] except it restricts the chain to a custom tip. The tip doesn't even need to be in the same chain as the tip. You can use this to find information about a point in the past or on a fork (if your chain oracle supports that).
I think after looking at it we don't need to do the Into<BlockId> thing because it's already convenient enough now that we have all these variants. I'd just take a BlockId. Should address @realeinherjar's complaint above.
Use the convenience methods wherever possible in wallet/mod.rs and in the tests etc

crates/bdk/src/wallet/mod.rs

crates/chain/tests/test_indexed_tx_graph.rs

crates/chain/tests/test_tx_graph.rs

example-crates/example_cli/src/lib.rs

SpaghettiSats · 2023-09-13T23:02:42Z

Thanks for doing this tedious work @SpaghettiSats. General comments:

1. No need to add specific tests for these. Try to avoid as much as possible changes to the test files in this PR. I think it's quite hard to implement these convenience methods incorrectly.

2. Docs need to explain what a subchain is. Liberally refer to the other method e.g. `subchain_X is like [X] except it restricts the chain to a custom tip`. The tip doesn't even need to be in the same chain as the tip. You can use this to find information about a point in the past or on a fork (if your chain oracle supports that).

3. I think after looking at it we don't need to do the `Into<BlockId>` thing because it's already convenient enough now that we have all these variants. I'd just take a `BlockId`. Should address @realeinherjar's complaint above.

4. Use the convenience methods wherever possible in `wallet/mod.rs` and in the tests etc

Thanks for the feedback. I'll work on these suggestions.

LLFourn

ACK be86650

Thanks @SpaghettiSats. I think another review from someone to confirm that these extra methods are worth maintaining for the convenience they offer.

LLFourn · 2023-09-19T01:24:37Z

crates/chain/src/tx_graph.rs

@@ -644,6 +644,11 @@ impl<A: Anchor> TxGraph<A> {

    /// Get the position of the transaction in `chain` with tip `chain_tip`.
    ///
+    /// This method is like [`try_get_chain_position`] except it restricts the
+    /// chain to a custom tip. The tip doesn't even need to be in the same chain as the tip.


Suggested change

/// chain to a custom tip. The tip doesn't even need to be in the same chain as the tip.

/// chain to a custom tip. The tip doesn't even need to be in the same chain as the current tip.

SpaghettiSats · 2023-09-19T08:38:15Z

I have rebased my branch and signed all the commits I have made, please if there are any other issues tell me and I will try to fix them.

danielabrozzoni

Thank you so much for this! I agree that removing the Default implementation on BlockId and instead using Option is much clearer, and I like the new methods that automatically fetch the chain tip from the chain oracle instead of requiring the user to provide one, they make the API way clearer.

However, I wonder whether it makes sense to have Option<BlockId> in some places. I left two comments about this, there are many other places where I'm left wondering the same thing, but I didn't comment on those as I'm probably just missing something :)

danielabrozzoni · 2023-09-29T11:05:09Z

crates/chain/tests/test_indexed_tx_graph.rs

@@ -469,5 +475,80 @@ fn test_list_owned_txouts() {
                confirmed: 80000          // tx1 + tx3
            }
        );
+
+        // Test that not specifing the chain_tip return the same results


025c4be is adding some tests that 7adfedb is removing, can you squash the two commits together instead?

danielabrozzoni · 2023-09-29T11:13:38Z

crates/chain/src/chain_oracle.rs

@@ -17,7 +17,7 @@ pub trait ChainOracle {
    fn is_block_in_chain(
        &self,
        block: BlockId,
-        chain_tip: BlockId,
+        chain_tip: Option<&BlockId>,


Mh, wouldn't it be better if we just had BlockId here? I can't really think of any case where someone would call is_block_in_chain(block, None), giving that it would always return None

danielabrozzoni · 2023-09-29T11:52:11Z

crates/chain/src/tx_graph.rs

        &self,
        chain: &C,
-        chain_tip: BlockId,
+        chain_tip: Option<BlockId>,


Same as my comment above, why having an Option here?

After a deeper look, I think we should keep this Option for 2 reasons: for consistency between methods and because it is convenient to check if the chain_tip is None in just one place instead of having to check every time before calling the method (the ChainOracle give an Option). If you think that is still the case to just pass the BlockId let me know and I will do it.

for consistency between methods

My point was that most (all?) of these methods should have a BlockId instead of an Option, I just commented a couple of them for brevity.

I see that it's convenient to have an Option here, but at the same time it makes the API a bit more difficult to understand.

Eventually it all comes down to personal preference, if you want to keep an Option here it's fine, but I think the decision should at least be documented properly

I think I agree with @danielabrozzoni here. It causes friction to call it with the output of get_chain_tip but that friction is born by us with this PR mostly so +1.

I get it, I'll fix the problem this weekend.

SpaghettiSats · 2023-10-02T07:56:30Z

First of all thank you for your review @danielabrozzoni. I agree with you. I will try to fix everything today.

…for BlockId

…p` of the passed `chain` instead of specifying one. refactor: remove unnecessary tests

…ckId>` for `chain_tip`

evanlinjin · 2023-10-08T14:33:09Z

Approach NACK

Thank you for taking this on and communicating clearly.

Overall, I agree that BlockId should not implement Default.

However, I think it is important that all TxGraph methods that need to fetch chain positions should still take in chain_tip: CheckPoint. Especially when multiple calls to these methods are made. The chain_tip ensures consistency in case of state changes of our ChainOracle.

Wallet::build_fee_bump is an example of how the new get_chain_position can potentially be misused. It is okay here because we are using LocalChain (which is in Wallet and we are holding a mutable reference over it). However, imagine if this is a ChainOracle implementation based on a client interface to a full/cbf node.

Additionally, now we have extra methods doing different but similar things. This increases the complexity of our API.

An Alternative Proposal:

I think we should require all ChainOracle implementations to be aware of the genesis block. LocalChain will no longer implement Default, and the caller needs to provide the genesis hash to construct it. The signature of ChainOracle::get_chain_tip would be changed to:

fn get_chain_tip(&self) -> Result<BlockId, Self::Error>;

So all TxGraph methods (which require getting chain positions) can take in chain_tip: CheckPoint (no Option), since we can guarantee to always at least have the genesis checkpoint.

The additional benefit to this, is if the caller accidentally updates LocalChain with data from the wrong network, the update can never connect (since the genesis block can't be replaced).

Related:

Create LocalChain with genesis block hardwired for Network #1079

LLFourn · 2023-10-09T00:12:34Z

Approach NACK

Thank you for taking this on and communicating clearly.

Overall, I agree that BlockId should not implement Default.

However, I think it is important that all TxGraph methods that need to fetch chain positions should still take in chain_tip: CheckPoint. Especially when multiple calls to these methods are made. The chain_tip ensures consistency in case of state changes of our ChainOracle.

I don't understand. They don't currently take in chain_tip: CheckPoint, they take a BlockId are you proposing to change this?

So to be clear, @evanlinjin's concern can be shown clearly in this change: https://github.com/bitcoindevkit/bdk/pull/1116/files#diff-ba2cf4a27dd1b461a84dc49953334c1efe322887febbcbe6f928057c36e7f7ec

The two calls should be using the same chain tip. They are because LocalChain has a lock on it during the two calls but we don't generally guarantee this for ChainOracle so if we were to switch it then it could come up with inconsistent answers. It's difficult to see how using an old chain tip for one call and then a newer chain tip for the next call is going to be worse than using an old chain tip for both calls -- but the concern is that users will be doing this without even knowing it.

Wallet::build_fee_bump is an example of how the new get_chain_position can potentially be misused. It is okay here because we are using LocalChain (which is in Wallet and we are holding a mutable reference over it). However, imagine if this is a ChainOracle implementation based on a client interface to a full/cbf node.

I imagined it but couldn't see what exactly the problem is. What's the concern?

Additionally, now we have extra methods doing different but similar things. This increases the complexity of our API.

Convenience methods do add maintenance complexity but reduce complexity for the user. I think it depends on whether the convenience methods send the right message. If people are meant to use a method 90% of the time in one way then it makes sense to have a convenience method that simplifies that for them and reassures them that they are not "missing out" on the right way of doing something by always using it that way. I thought that's what my suggestion was doing but it actually was not. You're meant to pass in the the same block id over multiple calls to these methods to keep the answers consistent but the convenience methods stops you from doing that. Perhaps we could include more nuanced documentation on the methods to make this clearer.

An Alternative Proposal:

I think we should require all ChainOracle implementations to be aware of the genesis block. LocalChain will no longer implement Default, and the caller needs to provide the genesis hash to construct it. The signature of ChainOracle::get_chain_tip would be changed to:
fn get_chain_tip(&self) -> Result<BlockId, Self::Error>;
So all TxGraph methods (which require getting chain positions) can take in chain_tip: CheckPoint (no Option), since we can guarantee to always at least have the genesis checkpoint.

The additional benefit to this, is if the caller accidentally updates LocalChain with data from the wrong network, the update can never connect (since the genesis block can't be replaced).

Related:
* [Create LocalChain with genesis block hardwired for Network #1079](https://github.com/bitcoindevkit/bdk/issues/1079)

I don't think this is really an alternative proposal -- it's compatible with this PR also.

evanlinjin · 2023-10-09T00:15:21Z

@LLFourn sorry I meant BlockId

LLFourn · 2023-10-09T00:20:40Z

To be clear in conclusion, I think we should not take this PR's approach for now and instead just wait on implementing @evanlinjin's proposal. I guess we can leave get_chain_tip on there. After further thinking I do see why it's useful even if we don't use it in our our API.

Sorry for originally suggesting this direction @SpaghettiSats. At least we have a better direction going forward. I think the Default on BlockId can be removed at the same time as requirement that LocalChain always be constructed with a genesis BlockId.

LLFourn · 2023-11-02T06:12:13Z

So since we are in fact going in the direction of #1079 let's close this. Once again sorry for the wasted work @SpaghettiSats. I wonder if BDK team can help prevent me from wasting other people's time like this. It seems I casually circumvented the issue validation and execution engine of the BDK team and it cost us.

An obvious suggestion is that we have ways of labeling issues that have actually been groomed by the core team that are ready for others to pick up.

Could you bring this up in the next dev meeting: @evanlinjin @notmandatory

notmandatory · 2023-11-13T16:36:11Z

I think for grooming new issues we should use the project "Status" field and only mark issues as "Todo" we're sure are ready and should be worked on.

realeinherjar reviewed Sep 11, 2023

View reviewed changes

LLFourn reviewed Sep 13, 2023

View reviewed changes

crates/bdk/src/wallet/mod.rs Outdated Show resolved Hide resolved

crates/chain/tests/test_indexed_tx_graph.rs Outdated Show resolved Hide resolved

crates/chain/tests/test_tx_graph.rs Outdated Show resolved Hide resolved

example-crates/example_cli/src/lib.rs Outdated Show resolved Hide resolved

LLFourn approved these changes Sep 19, 2023

View reviewed changes

SpaghettiSats force-pushed the master branch 2 times, most recently from 23fae02 to 16dcd95 Compare September 19, 2023 08:26

nondiremanuel mentioned this pull request Sep 23, 2023

W39 Lib Team Call bitcoindevkit/.github#78

Closed

40 tasks

nondiremanuel added this to the 1.0.0-alpha.3 milestone Sep 26, 2023

danielabrozzoni reviewed Sep 29, 2023

View reviewed changes

SpaghettiSats added 4 commits October 2, 2023 22:27

refactor: make chain_tip an Option and remove Default implementation …

f3a62c2

…for BlockId

refactor: implement new TxGraph methods that use the best `chain_ti…

ac54094

…p` of the passed `chain` instead of specifying one. refactor: remove unnecessary tests

refactor: set concrete type BlockId instead of generic `B: Into<Blo…

1f70465

…ckId>` for `chain_tip`

docs: improve chain tip definition

41ca63e

SpaghettiSats force-pushed the master branch 2 times, most recently from 5683ad3 to b5210e2 Compare October 2, 2023 20:30

refactor: remove a meaningless Option

a93ef52

SpaghettiSats force-pushed the master branch from b5210e2 to a93ef52 Compare October 2, 2023 20:52

notmandatory assigned SpaghettiSats Oct 3, 2023

nondiremanuel mentioned this pull request Oct 6, 2023

W41 Lib Team Call bitcoindevkit/.github#80

Closed

39 tasks

evanlinjin mentioned this pull request Oct 8, 2023

Create LocalChain with genesis block hardwired for Network #1079

Closed

LLFourn mentioned this pull request Oct 9, 2023

BlockId should not have a default and passing a chain tip should be an Option #1107

Closed

nondiremanuel modified the milestones: 1.0.0-alpha.3, 1.0.0-beta.0 Oct 10, 2023

LLFourn closed this Nov 2, 2023

notmandatory removed this from the 1.0.0-beta.0 milestone Nov 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BlockId should not have a default and passing a chain tip should be an Option #1107 #1116

BlockId should not have a default and passing a chain tip should be an Option #1107 #1116

SpaghettiSats commented Sep 11, 2023 •

edited

Loading

realeinherjar left a comment

realeinherjar Sep 11, 2023

LLFourn Sep 13, 2023

evanlinjin Sep 13, 2023

LLFourn Sep 14, 2023 •

edited

Loading

LLFourn left a comment •

edited

Loading

SpaghettiSats commented Sep 13, 2023

LLFourn left a comment

LLFourn Sep 19, 2023

SpaghettiSats commented Sep 19, 2023

danielabrozzoni left a comment

danielabrozzoni Sep 29, 2023

danielabrozzoni Sep 29, 2023

danielabrozzoni Sep 29, 2023

SpaghettiSats Oct 2, 2023

danielabrozzoni Oct 3, 2023

LLFourn Oct 5, 2023

SpaghettiSats Oct 6, 2023

SpaghettiSats commented Oct 2, 2023

evanlinjin commented Oct 8, 2023 •

edited

Loading

LLFourn commented Oct 9, 2023

evanlinjin commented Oct 9, 2023

LLFourn commented Oct 9, 2023

LLFourn commented Nov 2, 2023

notmandatory commented Nov 13, 2023 •

edited

Loading

	/// chain to a custom tip. The tip doesn't even need to be in the same chain as the tip.
	/// chain to a custom tip. The tip doesn't even need to be in the same chain as the current tip.

BlockId should not have a default and passing a chain tip should be an Option #1107 #1116

BlockId should not have a default and passing a chain tip should be an Option #1107 #1116

Conversation

SpaghettiSats commented Sep 11, 2023 • edited Loading

Description

Notes to the reviewers

Checklists

All Submissions:

New Features:

Bugfixes:

realeinherjar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LLFourn Sep 14, 2023 • edited Loading

Choose a reason for hiding this comment

LLFourn left a comment • edited Loading

Choose a reason for hiding this comment

SpaghettiSats commented Sep 13, 2023

LLFourn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SpaghettiSats commented Sep 19, 2023

danielabrozzoni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SpaghettiSats commented Oct 2, 2023

evanlinjin commented Oct 8, 2023 • edited Loading

LLFourn commented Oct 9, 2023

evanlinjin commented Oct 9, 2023

LLFourn commented Oct 9, 2023

LLFourn commented Nov 2, 2023

notmandatory commented Nov 13, 2023 • edited Loading

SpaghettiSats commented Sep 11, 2023 •

edited

Loading

LLFourn Sep 14, 2023 •

edited

Loading

LLFourn left a comment •

edited

Loading

evanlinjin commented Oct 8, 2023 •

edited

Loading

notmandatory commented Nov 13, 2023 •

edited

Loading