Inserting transactions in the Wallet should always include a last_seen.#1644
Closed
evanlinjin wants to merge 4 commits intobitcoindevkit:masterfrom
Closed
Inserting transactions in the Wallet should always include a last_seen.#1644evanlinjin wants to merge 4 commits intobitcoindevkit:masterfrom
Wallet should always include a last_seen.#1644evanlinjin wants to merge 4 commits intobitcoindevkit:masterfrom
Conversation
We rm `ConfirmationTime` because it is essentially the same thing as `ChainPosition<ConfirmationBlockTime>` without the block hash. We also impl `serde::Deserialize` and `serde::Serialize` for `ChainPosition`.
Previously, `Wallet::insert_tx` would insert a tx without a `last_seen` value. This meant that inserted transactions will not show up in the canonical history, nor be part of the UTXO set. Hence, new transactions created by the wallet may implicitly replace inserted transactions. This is somewhat unexpected for users. The new behavior of `insert_tx` inserts a tx with the current timestamp as `last_seen`. This, of course requires `std`. Therefore, we also introduced `insert_tx_at` which allows the caller to manually specify the `last_seen` timestamp. In addition, `test_insert_tx_balance_and_utxos` is removed since the behavior being tested is no longer expected.
This is no longer relevant as we direct callers to only insert tx into the wallet after a successful broadcast.
Not including a `seen_at` alongside the update means the unconfirmed txs of the update will not be considered to be part of the canonical history. Therefore, transactions created with this wallet may replace the update's unconfirmed txs (which is unintuitive behavior). Also updated the docs.
8 tasks
5 tasks
Member
Collaborator
Member
|
@oleonardolima good catch, I didn't notice a3d4eef was already merged. Closing for now and @evanlinjin please re-open if you don't agree all the commits here were already merged. |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Partially fixes #1642
Description
As mentioned in #1642,
Wallet::insert_txhas some confusing behavior. Callers typically expect that anything inserted into the wallet will not be replaced, and transaction inserted later will have precedence (in the case when we have conflicting txs).This PR modifies the behavior of
Wallet::insert_txto always add the current unix timestamp as thelast_seen-in-mempool value alongside the transaction being inserted. Since this is now astd-only method, we also introduce aWallet::insert_tx_atwhere thelast_seentimestamp is provided by the caller. We expect callers to only insert txs into the wallet after a successful broadcast. The documentation is updated to reflect this.Wallet::unbroadcast_transactionsis removed since callers are not expected to store unbroadcasted transactions in wallet anymore.In addition, the
seen_atinput is made mandatory forWallet::apply_update_at. Without this, callers may end up replacing unconfirmed txs when building txs.Notes to the reviewers
I don't think this is the perfect solution because a failed broadcast may be due to a network issue. In this case, the caller may need to persist the transaction external to wallet.
Changelog notice
Wallet::insert_txto always insert the tx alongside alast_seen-in-mempool timestamp (using the current timestamp).Wallet::insert_tx_at, which is a version oginsert_txwhere the user can explicitly set thelast_seentimestamp.Wallet::apply_update_atto always expect alast_seenvalue (not have it wrapped inOption).Wallet::unbroadcast_transactions.Checklists
All Submissions:
cargo fmtandcargo clippybefore committingBugfixes: