Add map_anchors for TxGraph

[yanganto]

Description

Fix #1295

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
• I'm linking the issue being fixed by this PR

[evanlinjin]

Thank you for this PR! What is the purpose of commit a223c0c?

[evanlinjin]

Thanks for this work. This will be a useful feature to have (talking from personal experience).

[yanganto]

Thank you for this PR! What is the purpose of commit a223c0c?

I see the doc

/// The generic `A` should be a [`Anchor`] implementation.

And it was not required in the enum, if it is required, we may not need an additional description on the document.

This is a chore, and not about this topic, so it is separated in another commit.

[evanlinjin]

@yanganto the key K for a BTreeSet in the stdlib is only useful if it impl's Ord, but it is not constrained in the struct definition (check here).

The reason for this is that generic constraints stack on top of each other and will start becoming tedious if types depend on types that have generics with constraints. So when we define anything, we avoid enforcing generic constraints wherever we can.

[yanganto]

@yanganto the key K for a BTreeSet in the stdlib is only useful if it impl's Ord, but it is not constrained in the struct definition (check here).

The reason for this is that generic constraints stack on top of each other and will start becoming tedious if types depend on types that have generics with constraints. So when we define anything, we avoid enforcing generic constraints wherever we can.

I think there is no Ord constrain on BTreeSet struct and there is no doc on the struct because the real constraint is from the impl, and the get, take, replace are the real constraints with Ord. I dropped the commit here and maybe dig into this later on. It is not a chore and out of this thread.

[yanganto]

@evanlinjin Does the test case look good from your end? If it is good, I will squash all the commit and resign it. 🙏

[evanlinjin]

@yanganto the key K for a BTreeSet in the stdlib is only useful if it impl's Ord, but it is not constrained in the struct definition (check here).

The reason for this is that generic constraints stack on top of each other and will start becoming tedious if types depend on types that have generics with constraints. So when we define anything, we avoid enforcing generic constraints wherever we can.

I think there is no Ord constrain on BTreeSet struct and there is no doc on the struct because the real constraint is from the impl, and the get, take, replace are the real constraints with Ord. I dropped the commit here and maybe dig into this later on. It is not a chore and out of this thread.

That's exactly my point. We should only put generic constraints where it's strictly necessary.

[yanganto]

There are more tx and blocks in the test case, and ready for review.

[evanlinjin]

ACK 5489f90

[yanganto]

Thanks for the review.

[LLFourn]

ACK 5489f90

I guess we don't want to add this method to IndexedTxGraph?. I think this is another reason to do #1147.

[LLFourn]

⛏️ This seems weird because we allocate a giant changeset and then just apply it. I think we can just repeat the process you do in ChangeSet.

[yanganto]

There are multiple anchor-related fields in TxGraph, and only one in ChangeSet. The function caller passed in possible non-determined, so it is good to map it once here to avoid issues from a non-determined function.

[LLFourn]

Huh isn't there only one acnhor related field on TxGraph also?

[evanlinjin]

Huh isn't there only one acnhor related field on TxGraph also?

We also have anchors in TxNode which should have 1 to 1 mapping to the anchors field. I think the implementation right now is safer and easier to read. If in the future, there is clear performance problems we can do something more efficient.

[evanlinjin]

I guess we don't want to add this method to IndexedTxGraph?. I think this is another reason to do #1147.

My gripe with #1147 is that I'm assuming the Indexer interface is too restrictive and I would like to think about it further before making it mandatory by including it in TxGraph. I.e. with Utreexo and for optimizing updating the "transaction/utxo view" of a purely block-by-block sync. For the second point, it will be good if the indexer returned which outputs and inputs are relevant straight away. This way, we can update our view of the UTXO set without reconstructing it from the TxGraph.

Otherwise, yes, we need map_anchors on IndexedTxGraph as well (for now).