Issue 954 #3
Closed
Issue 954 #3
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
0010ecd Add badge to README (wszdexdrf) 6904117 Change configs for source-base code coverage (wszdexdrf) Pull request description: ### Description This also changes the code coverage front end to coveralls instead of codecov, which had some issues with other changes in the PR. This will provide better and more accurate code coverage reports. ### Notes to the reviewers The tests run before generating the report are not exhaustive (not exhaustive earlier too, but I added as many as I could), and hence the report won't be 100% accurate. ### 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 ACKs for top commit: danielabrozzoni: ACK 0010ecd Tree-SHA512: 04a21b7481b80287cf8a31276238b0a5958871310664363f54d53779bb2dda6f49198baaf3b5471667fa6131443b022aabd86f2e3b1cfcf5d4aacadf137a166e
In this way we can check how much of a PR is covered by the tests, making the review process slightly easier.
- Remove default and minimal, as they are redundant - Use lexicographic order
aa0ea6a codecov: warn about missing features (Daniela Brozzoni) c3a7bbb codecov: slightly change the test features (Daniela Brozzoni) 1c4d478 Run code coverage on every PR (Daniela Brozzoni) Pull request description: ### 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 ACKs for top commit: notmandatory: ACK aa0ea6a afilini: ACK aa0ea6a Tree-SHA512: c2d0d9ad1e956f1c1808b46394f810939154442534241f9a023f18173910339ac236a5b6c66f78207dd2cb90a8f18000fc057b960e3354e2882c627fe1ef2c9f
PsbtUtils.fee_amount(), calculates the PSBT total transaction fee amount in Sats. PsbtUtils.fee_rate(), calculates the PSBT FeeRate, the value is only accurate AFTER the PSBT is finalized.
`del_tx` pulls the TransactionDetails object using `select_transaction_details_by_txid` method which gets the transaction details' data with a non-None transaction field even if the `include_raw` argument is `false`. So it becomes necessary to Set the transaction field in transactiondetails to None in `del_tx`, when we make a call to it with `include_raw=false`.
This PR aims to add more test to database code so that we can catch bugs as soon as they occur. Contributing to fixing issue bitcoindevkit#699.
Change parameter name in database test functions from `tree` to `db`.
fa998de Add psbt_signer.rs example (Steve Myers) Pull request description: ### Description Adding a simple example of how to create a PSBT with a watch only wallet and then sign it with a signing wallet. ### Notes to the reviewers This example was inspired by a question from a user. ### Changelog notice none. ### 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: * [ ] 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 ACKs for top commit: danielabrozzoni: ACK fa998de afilini: ACK fa998de Tree-SHA512: 2e178ee59ce94eb1e9b5616a499e106e1d2843589036bdb6cff2c987e280588ad9989b026cdbf01290cc60f02eccbc410a3e1d1cd45eb4b8ff04353ae31b23ea
… PsbtUtils trait ab41679 Add fee_amount() and fee_rate() functions to PsbtUtils trait (Steve Myers) Pull request description: ### Description The purpose of the PR is to provide a more convenient way to calculate the transaction fee amount and fee rate for a PSBT. This PR adds `fee_amount` and `fee_rate` functions to the existing `PsbtUtils` trait and implements them for `PartiallySignedBitcoinTransaction`. The `fee_rate` value is only valid if the PSBT it is called on is fully signed and finalized. See related discussion: bitcoindevkit/bdk-ffi#179 ### Changelog Added - PsbtUtils.fee_amount(), calculates the PSBT total transaction fee amount in Sats. - PsbtUtils.fee_rate(), calculates the PSBT FeeRate, the value is only accurate AFTER the PSBT is finalized. ### Notes to the reviewers Ideally I'd like `fee_rate` to return an `Option` and return `None` if the PSBT isn't finalized. But I'm not quite sure how to determine if a PSBT is finalized without having a `Wallet` and running it through the finalize code first. Or there might be a way to fill in missing signatures with properly sized fake data prior to calculating the fee rate. For now I think it's enough to do this simple approach with usage warning in the rust docs. ### 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: afilini: ACK ab41679 Tree-SHA512: 5386109c9ffcf63160f18b4c51eb2c582f4121b669c7276aaba489d186cf9b97343d46f887469b1407ccd7a24448b5e7aee4c6832d86768239b21f4e68054a0f
8963e8c Improve docs w.r.t. PSBT finalization (Elias Rohrer) Pull request description: ### Description This PR includes just a few tiny changes to the docs trying to make it a bit more clear what PSBT finalization is and what to expect. ### Notes to the reviewers ### 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 ACKs for top commit: afilini: ACK 8963e8c Tree-SHA512: ea1a6c715c2832cdf3a428654fb8fad08a0549e46a808ca7a424590145fc2c40164ea08eb636ce5cd648dd6335480accd1d91f9e37e9397feaf0dac2015e8baa
Change the compiler clap app into a specific example. Add comment docs and example description. Remove clap from dependency.
Add a new policy example demonstrating the bdk's policy structure and how to derive it for any descriptor without creating a bdk wallet.
The current implementation of the `migrate` method for Sqlite database does not rollback changes when there is an error while running one of the migration scripts. This can leave the database in an inconsistent state. This change ensures that migrations either succeed completely or fail.
Some wallets may only specify the `non_witness_utxo` for a PSBT input. If that's the case, BDK should still be able to sign. This was pointed out in the discussion of bitcoindevkit#734
e65edbf Change parameter name of database in test funcs (Vladimir Fomene) 8830704 Add more test to the database module (Vladimir Fomene) e06c3f9 Set tx field to none if `include_raw` is false (Vladimir Fomene) Pull request description: ### Description This PR add more test to the database module and also fixes certain bugs discovered by the written test. I also amended the name used for the database parameter in the test functions. ### Notes to the reviewers This contributes to fixing bitcoindevkit#699 ### Changelog notice <!-- Notice the release manager should include in the release tag message changelog --> <!-- See https://keepachangelog.com/en/1.0.0/ for examples --> ### 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 ACKs for top commit: rajarshimaitra: tACK e65edbf danielabrozzoni: Code review ACK e65edbf Tree-SHA512: 1ac1475f7d63f25e94ef21342e6f6e243c34c8c9208d11a5492f224026055da2a96f20be83497c1ba361effff9861f4e68920f98feebaf4b201d205c7030c282
…y `non_witness_utxos` 5e9965f Enable signing taproot transactions with only `non_witness_utxos` (Alekos Filini) Pull request description: ### Description Some wallets may only specify the `non_witness_utxo` for a PSBT input. If that's the case, BDK should still be able to sign. This was pointed out in the discussion of bitcoindevkit#734 ### Changelog notice - Enable signing taproot transactions that only specify the `non_witness_utxo` ### 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 #### Bugfixes: * [ ] This pull request breaks the existing API * [x] I've added tests to reproduce the issue which are now passing * [x] I'm linking the issue being fixed by this PR ACKs for top commit: danielabrozzoni: tACK 5e9965f - the code looks good to me, I played around with the test you provided (inspecting the PSBT, adding/removing the witness and non-witness utxos, etc) and everything works as expected. Tree-SHA512: 2f205286263bfee4c76de8e8c81ae1349b1c3b255b72045488f8d629c05cab64c6f775307e831674dc036e5a3a760f95d9cdc1beaf48afb4c475aee838131a33
54d7684 Sqlite migrations should either succeed or fail (Vladimir Fomene) 369e17b Add datatype for is_spent sqlite column (Vladimir Fomene) Pull request description: ### Description During table creation, Sqlite does not throw an error when a column datatype is not defined. In addition, the datatype provided during table creation does not put a constraint on the type of data that can be put in that column. So you can easily put a string value in an integer column. Despite this, I think it is important for us to add the datatype for clarity. ### Notes to the reviewers You can read more about how Sqlite dynamic typing [here](https://www.sqlite.org/faq.html). I have amended our `migrate` code with a new commit. The idea is to run migrations in a transaction so that they either succeed or fail. This prevents us from having the database in an inconsistent state at any point in time. ### 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 #### 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 ACKs for top commit: rajarshimaitra: tACK 54d7684 afilini: ACK 54d7684 Tree-SHA512: bb6c0467f799ca917f8d45c6495b766352b3177fc81952fcdd678208abf092fdeae966686528a5dcb3f342d7171783274df6312a08cbef3580063e059f5f7254
This check is redundant since it's already performed by miniscript (see https://docs.rs/miniscript/7.0.0/miniscript/miniscript/analyzable/enum.AnalysisError.html#variant.RepeatedPubkeys) and it was incorrectly failing on tr descriptors that contain duplicated keys across different taproot leaves Fixes bitcoindevkit#760
97b6fb0 Add a policy example (rajarshimaitra) da76708 Update compiler example. (rajarshimaitra) Pull request description: ### Description Fixes bitcoindevkit#729. There is an "unmaintained" warning in a old version of clap, which triggered the issue. Ideally, we should not have clap in bdk's dependency. It was only used for the `compiler.rs` example, which was a very tiny clap app compiling miniscript policies, and it wasn't really an example for bdk. This PR rewrites the example as a `policy.rs` which demos the BDK's Policy module and policy structures. - Use a `wsh(multi(2, Privkey, Pubkey))` descriptor, which has only one part private and other part public. - use `into_wallet_descriptor()` to turn that into a `Descriptor` and `KeyMap`. - Use the `KeyMap` to create a custom signer. - Extract the descriptor `Policy` structure from the given keymap. I am not very sure on how much this example is helpful. I still find it hard to read the Policy structure visually. But if Policy is something we want the user to know about descriptors and bdk wallets, this shows how to extract it for a simple multisig condition. Note: There is no use of `bdk::wallet` in the example. BDK uses the Policy extraction internally while transaction creation. But all these are exposed publicly, so can be used independently too. ### Questions: - Should we still have a `minscript::policy::compile()` example? Which IIUC is very different from `bdk::policy:Policy`. I didn't include it in this PR, because I am not sure if it fits inside bdk example categories. - Should we expose `extract_policy` as an wallet API? All though its possible to get policy without creating a wallet, why not let the wallet also spit one out for itself, if its useful? ### 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 ACKs for top commit: afilini: ACK 97b6fb0 Tree-SHA512: 8e3719fdad308a347d22377050b2f29e02a884ff7d4e57a05a06d078de0709b5bf70bbcb1a696d1e1cdfe02cdb470e5af643da7c775b67fe318046bd6b80f440
e2bf973 Remove redundant duplicated keys check (Alekos Filini) Pull request description: ### Description This check is redundant since it's already performed by miniscript (see https://docs.rs/miniscript/7.0.0/miniscript/miniscript/analyzable/enum.AnalysisError.html#variant.RepeatedPubkeys) and it was incorrectly failing on tr descriptors that contain duplicated keys across different taproot leaves Fixes bitcoindevkit#760 ### 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 #### Bugfixes: * [x] This pull request breaks the existing API * [x] I've added tests to reproduce the issue which are now passing * [x] I'm linking the issue being fixed by this PR ACKs for top commit: danielabrozzoni: Code review ACK e2bf973 - the code looks good to me, but I didn't test manually Tree-SHA512: 3c557d741e34abf6336c7e257867f2c6f91a4be0024317af834f08ba7c0c64557bd74b643005254c9f04e953350b104b0d4d287f0d0528134b357a4adf580f87
e1fa0b6 Fix the new release process (Alekos Filini) Pull request description: ### Description A few things I noticed while doing the 0.22 release. Follow-up of bitcoindevkit#544 ### 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) ACKs for top commit: notmandatory: ACK e1fa0b6 Tree-SHA512: 5e60e73ba1f820fc39f62b75583e1125f911e578ab7263a20636e2a995d0efa10ba1b3b66a1e03d8a2ed61e32c00b53de9d6bbdb31de94db37c79fa5acdbf483
This commit contains a change to address issue bitcoindevkit#752 cargo test runs successfully.
e6f2d02 Remove genesis_block lazy initialization (Shobit Beltangdy) Pull request description: ### Description This commit contains a change to address issue bitcoindevkit#752 cargo test runs successfully. ### Notes to the reviewers Hi, newbie here learning Rust and BDK! I've removed the lazy_static block in this commit, and when learning about lazy_static also came across something called [once_cell](https://doc.rust-lang.org/std/cell/struct.OnceCell.html), [soon to be available in stdlib](rust-lang/rust#74465). It's like lazy_static but faster and is not a macro. Shall I keep the lazy_static and create an issue to switch to once_cell in the future, or remove the lazy_static (as I've done) and create an issue anyway? #### 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 ACKs for top commit: notmandatory: ACK e6f2d02 Tree-SHA512: 528f5fdfb0d7d1f7a83869b7a0de1b25dfcfafae2671c9229cdb4e5d80d11e5578d9325b3d95555e59f5dfb4ed6304c0c112a01a56b6a596e50dd62e310c516e
Remove the requirement that evicted blocks should have in-best-chain counterparts in the update.
It is better to have this external to this structure.
`tx_graph.rs` is rearranged as well.
ebd6103 Fix policy condition calculation (Alekos Filini) Pull request description: ### Description When constructing the `Condition` struct we recursively call `get_condition` on all the items in a threshold and short-circuit if there's an error somewhere (for example, because the policy-path hasn't been provided for a specific threshold). This can cause issues when the user doesn't care about a subtree, because we still try to call `get_condition` on all the items and fail if something is missing, even if the specific subtree isn't selected and won't be used later on. This commit changes the logic so that we first filter only the `selected` items, and then unwrap the error using the question mark. If errors happened somewhere else they will be ignored, as they should. ### Notes to the reviewers I think it makes sense to backport this to `0.27`: even though it's not a critical issue (and there's a workaround[^1] for the bug) it may be a while before the new `1.0` is released. I wouldn't do a release just for this, but I would just leave it there and maybe in a few weeks if there are other fixes to be backported to pre-1.0 they could all be released. ### Changelog notice - Fixed a bug in the policy condition calculation ### 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: * [ ] I've added tests for the new feature * [ ] I've added docs for the new feature #### Bugfixes: * [ ] This pull request breaks the existing API * [x] I've added tests to reproduce the issue which are now passing * [ ] I'm linking the issue being fixed by this PR [^1]: The workaround is to simply set the items in the policy tree even if they won't be used. For example, if the item causing troubles is a `thresh(1, ...)` just set `[0]` in the policy path for that id. ACKs for top commit: notmandatory: ACK ebd6103 Tree-SHA512: 1e0c6140e47ead3a518656cab660932b40f9e67425ce144f072267c30fd5cae5923ba943d9682af050c8f314756c522260abb68b104b9eeaae597319612984db
Previously, I have misunderstood the definition of anchor. If a tx is anchored in a block, it does not necessarily mean it is confirmed in that block. The tx can be confirmed in an ancestor block of the anchor block. With this new definition, we need a new trait `ConfirmationHeight` that has one method `confirmation_height`. This trait can be used to extend `Anchor` for those implementations that can give us the exact conirmation height of a tx (which is useful in most cases). Another change is to add another variant to the `ObservedAs` enum; `ObservedAs::ConfirmedImplicit(A)`. If a tx does not have an anchor, but another tx that spends it has an anchor that in in the best chain, we can assume that tx is also in the best chain. The logic of `TxGraph::try_get_chain_position` is also changed to reflect this. Some methods from `IndexedTxGraph` have been moved to `TxGraph` as they do not require the `Indexer`. Some `TxGraph` methods have been renamed for clarity and consistency. Also more docs are added.
`ObservedAs::ConfirmedImplicit` is incomplete, remove for now. `local_chain::ChangeSet` does not need to be a single-element tuple struct.
This is mostly copying over the relevant tests from `SparseChain`. Changes are made to `local_chain::ChangeSet` to re-add the ability to remove blocks.
The `HashSet` was used for iterating without duplicate items. However, since `anchors` is a `BTreeSet`, heights are in order. So a single variable tracking last height will be sufficient.
* Remove `chain_oracle::CacheBackend` for now as it is not used. * `SparseChain` does not need to implement `ChainOracle`. * Remove filter predicate for `list..` methods of `TxGraph` and `IndexedTxGraph` as this is premature optimisation. * `Append` can be implemented for all `BTreeMap`s and `BTreeSet`s, instead of only `local_chain::ChangeSet`.
The `ConfirmationHeight` trait has been removed in favour of a second method on the `Anchor` trait: `confirmation_height_upper_bound()`. Methods `try_balance_at()` and `balance_at()` of `IndexedTxGraph` have been removed as they do not provide additional functionality. `IndexedTxGraph::insert_relevant_txs` now uses two loops, the first loop indexes all transactions first. This is done as some indexes require ancestor transactions to be indexed first and we cannot guarantee that the input transactions are in topological order.
Ensure `insert_relevant_txs` does not require transactions to be in topological order. Other changes: Rm `try_list_owned_txs` as it is useless
The `insert_relevant_txs` test has also been changed to used `KeychainTxOutIndex` so that index additions can be checked (`SpkTxOutIndex` has no additions). Additionally, generic bounds of some `IndexedTxGraph` list methods have been fixed.
…able` Docs are updated to explain why `is_mature` and `is_confirmed_and_spendable` may return false-negatives.
Instead of forcing all transactions inserted to use the same anchors, we change the API to have unique anchors per transaction. This allows for more flexibility in general. For example, use `Anchor` implementations that contain the position in a block of a transaction.
* `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.
b799a57 [bdk_chain_redesign] Add tests for `IndexedTxGraph` with `LocalChain` (rajarshimaitra) 8cd0328 [bdk_chain_redesign] Implement `OwnedIndexer` for indexers (rajarshimaitra) 911af34 [bdk_chain_redesign] Fix calculation bugs. (rajarshimaitra) e536307 [bdk_chain_redesign] Fix `tx_graph::Additions::append` logic (志宇) f101dde [bdk_chain_redesign] Fix `tx_graph::Additions::append` logic (志宇) 1b15264 [bdk_chain_redesign] Change `insert_relevant_txs` method (志宇) ecc74ce [bdk_chain_redesign] Docs for `is_mature` and `is_confirmed_and_spendable` (志宇) ac336aa [bdk_chain_redesign] Make `insert_relevant_txs` topologically-agnostic (志宇) 165b874 [bdk_chain_redesign] Add test for `insert_relevant_txs` (志宇) f3e7b67 [bdk_chain_redesign] Various tweaks and fixes (志宇) 03c1283 [bdk_chain_redesign] Revert changes to `SparseChain` (志宇) 34a7bf5 [bdk_chain_redesign] Rm unnecessary code and premature optimisation (志宇) 6c49570 [bdk_chain_redesign] Rm `HashSet` from `TxGraph::relevant_heights` (志宇) 1003fe2 [bdk_chain_redesign] Test `LocalChain` (志宇) 7175a82 [bdk_chain_redesign] Add tests for `TxGraph::relevant_heights` (志宇) 8e36a2e [bdk_chain_redesign] Remove incomplete logic (志宇) 81436fc [bdk_chain_redesign] Fix `Anchor` definition + docs (志宇) 001efdd Include tests for new updates of TxGraph (rajarshimaitra) 10ab77c [bdk_chain_redesign] MOVE `TxIndex` into `indexed_chain_graph.rs` (志宇) 7d92337 [bdk_chain_redesign] Remove `IndexedTxGraph::last_height` (志宇) a7fbe0a [bdk_chain_redesign] Documentation improvements (志宇) ee1060f [bdk_chain_redesign] Simplify `LocalChain` (志宇) 611d2e3 [bdk_chain_redesign] Consistent `ChainOracle` (志宇) bff80ec [bdk_chain_redesign] Improve `BlockAnchor` docs (志宇) 24cd8c5 [bdk_chain_redesign] More tweaks and renamings (志宇) ddd5e95 [bdk_chain_redesign] Modify signature of `TxIndex` (志宇) da4cef0 [bdk_chain_redesign] Introduce `Append` trait for additions (志宇) 89cfa4d [bdk_chain_redesign] Better names, comments and generic bounds (志宇) 6e59dce [bdk_chain_redesign] `chain_oracle::Cache` (志宇) a7eaebb [bdk_chain_redesign] Add serde support for `IndexedAdditions` (志宇) c09cd2a [bdk_chain_redesign] Added methods to `LocalChain` (志宇) 7810059 [bdk_chain_redesign] `TxGraph` tweaks (志宇) a63ffe9 [bdk_chain_redesign] Simplify `TxIndex` (志宇) a1172de [bdk_chain_redesign] Revert some API changes (志宇) 8c90617 [bdk_chain_redesign] Make default anchor for `TxGraph` as `()` (志宇) 468701a [bdk_chain_redesign] Initial work on `LocalChain`. (志宇) 34d0277 [bdk_chain_redesign] Rm anchor type param for structs that don't use it (志宇) 3440a05 [bdk_chain_redesign] Add docs (志宇) 236c50f [bdk_chain_redesign] `IndexedTxGraph` keeps track of the last synced height (志宇) e902c10 [bdk_chain_redesign] Fix `apply_additions` logic for `IndexedTxGraph`. (志宇) 313965d [bdk_chain_redesign] `mut_index` should be `index_mut` (志宇) db7883d [bdk_chain_redesign] Add balance methods to `IndexedTxGraph` (志宇) d0a2aa8 [bdk_chain_redesign] Add `apply_additions` to `IndexedTxGraph` (志宇) 6cbb18d [bdk_chain_redesign] MOVE: `IndexedTxGraph` into submodule (志宇) 784cd34 [bdk_chain_redesign] List chain data methods can be try/non-try (志宇) 43b648f [bdk_chain_redesign] Add `..in_chain` methods (志宇) 61a8606 [bdk_chain_redesign] Introduce `ChainOracle` and `TxIndex` traits (志宇) 5ae5fe3 [bdk_chain_redesign] Introduce `BlockAnchor` trait (志宇) Pull request description: ### Description This is part of bitcoindevkit#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 new`TxGraph` 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:** * ~@evanlinjin: The `TxIndex` trait should not have `is_{}_relevant` methods as we cannot guarantee this across all transaction indexes. We should introduce extension traits for these (bitcoindevkit#926 (comment) * ~@evanlinjin: `BlockAnchor` should be defined as "if this block is in chain, then this tx must be in chain". Therefore, the anchor does not provide us with the exact confirmation height of the tx. We need to introduce an extension trait `ExactConfirmationHeightAnchor` for certain operations (bitcoindevkit#926 (comment) **What will be done external to this PR:** * @rajarshimaitra: Persisting `indexed_tx_graph::Additions` (bitcoindevkit#937). * @notmandatory: Update examples to use redesigned structures. * Update `bdk::Wallet` to use the redesigned structures. ### Changelog notice * Initial implementation of the `bdk_chain` redesign (as mentioned in bitcoindevkit#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: * [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: rajarshimaitra: ACK bitcoindevkit@b799a57 danielabrozzoni: 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. Tree-SHA512: 8c386354cbd02f0701b5134991b65d206a54d19a2e78ab204e6ff1fa78a18f16299051bc0bf4ff4d2f5a0adab9b15658fa53cd0de2ca16969f4bf6a485225082
|
Thanks. can you rebase this onto master please. bitcoindevkit#926 got merged. |
|
Can you please review the PR? @LLFourn |
|
Oops sorry @Shourya742 I forgot to include in my original request: can you rebase and make the PR to bitcoindevkit/bdk master rather than @evanlinjin's fork. Cheers. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Additions structure field "tx" is changed to "txs" and relevant changes are made to all files using it.
Issue link: bitcoindevkit#954 (comment)
Notes to the reviewers
Checklists
All Submissions:
cargo fmtandcargo clippybefore committingBugfixes: