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.

Sign up for GitHub

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

Jump to bottom

Furthur changes to redesign structures. #971

Closed
Tracked by #57
, #59
...
evanlinjin opened this issue May 5, 2023 · 5 comments
Closed
Tracked by #57
, #59
...

Furthur changes to redesign structures. #971

evanlinjin opened this issue May 5, 2023 · 5 comments
Assignees
@evanlinjin
Labels
new feature New feature or request
Milestone
1.0.0-alpha.1

Comments

@evanlinjin
Copy link
Member

evanlinjin commented May 5, 2023 •
edited
Loading

Describe the enhancement

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

  1. 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".
    -- Completed by Implement persistence with the new structures #965

  2. 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.

  3. OwnedIndexer is bad. Here is something better:

    pub trait OwnedTxOuts {
    type SpkIndex;
    type 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.
    -- Turns out OwnedTxOuts is not needed either, all removed in Improve txout listing and balance APIs for redesigned structures #975
@evanlinjin evanlinjin added new feature New feature or request BDK 1.0.0 labels May 5, 2023
@evanlinjin evanlinjin added this to the 1.0.0-alpha.1 milestone May 5, 2023
@evanlinjin evanlinjin self-assigned this May 5, 2023
@evanlinjin evanlinjin added this to BDK May 5, 2023
@notmandatory notmandatory moved this to Todo in BDK May 6, 2023
@notmandatory notmandatory mentioned this issue May 9, 2023
Closed
21 tasks
@evanlinjin evanlinjin mentioned this issue May 9, 2023
Merged
5 tasks
@notmandatory notmandatory moved this from Todo to In Progress in BDK May 9, 2023
@notmandatory notmandatory moved this from In Progress to Todo in BDK May 9, 2023
evanlinjin added a commit that referenced this issue May 10, 2023
@evanlinjin
Merge #965: Implement persistence with the new structures
05d353c 
4963240 Add more `impl`s for `Append` and docs for file store `magic` (志宇)
2aa08a5 [persist_redesign] Introduce redesigned `persist` types (志宇)

Pull request description:

  ### Description

  This is part of #895 and #971

  * Introduce a more generic version of the `keychain::persist::*` structures that only needs a single generic for the changeset type.

  Additional changes:

  * The `Append` trait has a new method `is_empty`.
  * Introduce `Store` structure for `bdk_file_store` (which implements `PersistBackend`).

  ### Changelog notice

  ### Checklists

  #### All Submissions:

  * [x] I've signed all my commits
  * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md)
  * [x] I ran `cargo fmt` and `cargo clippy` before committing

  #### New Features:

  * [x] I've added tests for the new feature
  * [x] I've added docs for the new feature

Top commit has no ACKs.

Tree-SHA512: 0211fbe7d7e27805d3ed3a80b42f184cdff1cebb32fd559aa9838e4a7f7c7e47b6c366b6ef68e299f876bafed549b8d1d8b8cc0366bf5b61db079504a565b9b4
@evanlinjin
Copy link
Member Author

For point number 3, I think neither the OwnedIndexer or OwnedTxOuts traits are needed. Will have a PR soon.

@evanlinjin evanlinjin mentioned this issue May 10, 2023
Merged
5 tasks
evanlinjin added a commit that referenced this issue May 11, 2023
@evanlinjin
Merge #975: Improve txout listing and balance APIs for redesigned s…
34d6087 
…tructures

ed89de7 Improve txout filter/listing method docs for `TxGraph` (志宇)
fb75aa9 Clarify `TxGraph::try_filter_chain_unspents` logic (志宇)
96b1075 Fix and improve `Persist::commit` method (志宇)
e01d17d Improve `txout` listing and balance APIs for redesigned structures (志宇)

Pull request description:

  ### Description

  As noted in #971 (comment).

  Instead of relying on a `OwnedIndexer` trait to filter for relevant txouts, we move the listing and balance methods from `IndexedTxGraph` to `TxGraph` and add an additional input (list of relevant outpoints) to these methods.

  This provides a simpler implementation and a more flexible API.

  #### Other Fixes

  The `Persist::commit` method is fixed in 96b1075.

  Previously, regardless of whether writing to persistence backend is successful or not, the logic always cleared `self.staged`. This is changed to only clear `self.staged` after successful write.

  Additionally, the written changeset (if any) is returned, and `PersistBackend::write_changes` will not be called if `self.staged` is empty.

  ### Notes to the reviewers

  Yes, slightly more boilerplate to do the same things, but less code to maintain and a much more flexible API. Very worth it IMO.

  ### Changelog notice

  ### Checklists

  #### All Submissions:

  * [x] I've signed all my commits
  * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md)
  * [x] I ran `cargo fmt` and `cargo clippy` before committing

  #### New Features:

  * [x] I've added tests for the new feature
  * [x] I've added docs for the new feature

ACKs for top commit:
  LLFourn:
    ACK ed89de7

Tree-SHA512: efae18c13ee74eff801febca61cb16bf4d8f3203ced96172b9ef555d8d2a749a382f87d024e8c92aacbab7eb4e50a8337de798e10524291ad80c6b35c4dc0161
@nondiremanuel nondiremanuel moved this from Todo to Done in BDK May 11, 2023
@nondiremanuel nondiremanuel moved this from Done to In Progress in BDK May 11, 2023
@notmandatory notmandatory mentioned this issue May 15, 2023
Closed
19 tasks
@evanlinjin
Copy link
Member Author

I don't know whether 2. is actually needed atm.

@notmandatory notmandatory moved this from In Progress to Needs Review in BDK Jun 2, 2023
@notmandatory
Copy link
Member

Should be fixed in #976

@notmandatory
Copy link
Member

@evanlinjin can we close this now that #976 is merged?

@evanlinjin
Copy link
Member Author

Fixed in #976

@github-project-automation github-project-automation bot moved this from Needs Review to Done in BDK Jun 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@evanlinjin evanlinjin

Labels
new feature New feature or request
Projects
BDK
Archived in project
Milestone
1.0.0-alpha.1
Development

No branches or pull requests
2 participants
@notmandatory @evanlinjin