[wallet] Track conflicted transactions removed from mempool and fix UI notifications #18600
Conversation
82f363f to
b34318d
Compare
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
There was a problem hiding this comment.
Nice to see progress on this!
Started review (will update list below with progress).
- 9eb9af6 [validationinterface] Extend TransactionRemovedFromMempool with fIsConflicted (1/4)
- 497c502 [wallet] Track transactions removed from mempool due to conflicts (2/4)
- 1af561a [rpcwallet] Add
conflictedas transaction description field (3/4) - b34318d [functional] Test conflict on non-connected wallet transactions (4/4)
src/interfaces/chain.cpp
Outdated
| @@ -158,7 +158,7 @@ class NotificationsProxy : public CValidationInterface | |||
| { | |||
| m_notifications->transactionAddedToMempool(tx); | |||
| } | |||
| void TransactionRemovedFromMempool(const CTransactionRef& tx) override | |||
| void TransactionRemovedFromMempool(const CTransactionRef& tx, bool fIsConflicted) override | |||
There was a problem hiding this comment.
In commit "[validationinterface] Extend TransactionRemovedFromMempool with fIsConflicted" (9eb9af6)
Should just call this argument conflicted or is_conflicted
There was a problem hiding this comment.
Maybe even just pass the original removal reason to the sink? Compressing REPLACED into a boolean called conflicted is not wrong, but maybe not the most straightforward either.
There was a problem hiding this comment.
Yes I hesitated at first to call it is_conflicted but it's still an implementation of a CValidationInterface method so fIsConflicted is more reasonable so is_ibd should be fInitialDownload ?
@MarcoFalke, you suggest doing the filtering in NotificationsProxy ? Shouldn't this be as lean as possible and just call notifications handler ? My motivation was avoiding linking mempool code in wallet but didn't think specifically about this point.
There was a problem hiding this comment.
I don't think you need to link any mempool code when you want access to the enum RemovalReason in the wallet.
Right? cc @ryanofsky
There was a problem hiding this comment.
re: #18600 (comment)
Yes I hesitated at first to call it is_conflicted but it's still an implementation of a
CValidationInterfacemethod sofIsConflictedis more reasonable sois_ibdshould befInitialDownload?
Just following the naming conventions is "preferred in new code". We try to take a long term view. Eventually things will be updated to follow the conventions, and following them when it's possible and the cost is low helps get there quicker.
@MarcoFalke, you suggest doing the filtering in
NotificationsProxy? Shouldn't this be as lean as possible and just call notifications handler ? My motivation was avoiding linking mempool code in wallet but didn't think specifically about this point.
Would worry more about semantics than linking here. Link errors are easy to detect and fix. An ambiguous API can lead to confusion and bugs and workarounds that never fix the original problem. If it makes semantic sense for the mempool to have different definitions of "conflicted" internally and externally and to translate reason == CONFLICT or reason == REPLACED to conflicted == true, then this is API is fine. If it makes more sense to use consistent reason codes everywhere, this should just pass the codes.
In terms of building and linking, it's fine for wallet code to include txmempool.h and share the enum data type. Linker will only complain if the wallet is calling mempool functions. src/interfaces code could forward decl
10000
are enum class MemPoolRemovalReason; for now, and longer term if the interface stabilizes the actual enum definition could move there.
There was a problem hiding this comment.
I restrained conflict to block only, not including anymore mempool replacement which means API is far-more straightforward as wallet conflict == mempool conflict.
If we really want to use consistent reason codes among clients, we may pull them out of txmempool.h in some node/*.h ?
There was a problem hiding this comment.
re: #18600 (comment)
I restrained conflict to block only
Thanks, this seems ok now that the definition of conflicted here matches up with the MemPoolRemovalReason definition. I do think the API would be more useful and clear if it just passed the existing MemPoolRemovalReason value instead introducing a new way to communicate the same information. But thanks for resolving the original concern!
If we really want to use consistent reason codes among clients, we may pull them out of txmempool.h in some node/*.h ?
I suggested moving it to src/interfaces/ in #18600 (comment) because src/interfaces/ is meant to be a home for types used at the node/wallet boundary. But src/node/ would be also ok, and leaving it in txmempool.h is perfectly fine. I would leave it in txmempool.h if using it in this PR, because this PR is already doing a lot of things. The current source code organization is not ideal and is gradually being improved, so design of the interfaces::Chain::Notifications::transactionRemovedFromMempool API shouldn't be decided based on what source file an enum definition is located in. The design should try to be useful and consistent and clear, and source code organization should follow the design instead of the other way around
There was a problem hiding this comment.
Yes I agree on source code organization following the design. I think MemPoolRemovalReason may be dry-ed up in itself, IIRC it's not used beyond ::CONFLICTED. But outside PR scope.
| LOCK(cs_wallet); | ||
| auto it = mapWallet.find(ptx->GetHash()); | ||
| if (it != mapWallet.end()) { | ||
| it->second.fInMempool = false; | ||
| } | ||
| if (fIsConflicted) { | ||
| CWalletTx::Confirmation confirm(CWalletTx::Status::CONFLICTED, /* block_height */ 0, /* block_hash */ {}, /* nIndex */ 0); |
There was a problem hiding this comment.
In commit "[rpcwallet] Add conflicted as transaction description field" (1af561a)
I think this should probably be Status::UNCONFIRMED rather than Status::CONFLICTED. That should restore the original behavior before #16624 (a31be09 + 7e89994) and avoid introducing an entirely new transaction state that wallet code has never had to deal with, where a transaction is conflicting with something not in a specific block. This should also allow reverting GetDepthInMainChain below and leaving it unchanged.
At a high level I think it makes sense to mark these transactions unconfirmed instead of conflicted because we know there are cases the wallet can't detect unconfirmed transactions becoming conflicted, and cases where it can't detect a conflicting transactions becoming unconfirmed, and unconfirmed is the more conservative choice than conflicted for balances, etc
I haven't looked at your wallet_txn_doublespend.py test yet, so I'm not sure how changing this would interact that. Another way of testing this code which might be better from the point of view of #18600 is to add a regression test to feature_notifications.py to ensure that this commit restores the -walletnotify notifications that were lost in #16624
There was a problem hiding this comment.
Thanks for your comment, I didn't reverse to the previous behavior starting from the user viewpoint. Unconfirmed funds allow you to spend from then or consider likely-yours as part of your balance, but here we know, as of mempool state, they are conflicted and likely-but-not-certain not going to confirm. We don't want the user to take decisions based on funds he has not. You can spend from unconfirmed, right ?
Knowing there are cases the wallet can't detect unconfirmed transactions becoming conflicted doesn't mean that when we have the information available we shouldn't act on this.
Thanks for feature_notifications.py, we will modify to test notifications, didn't know its existence.
There was a problem hiding this comment.
re: #18600 (comment)
Thanks for your comment, I didn't reverse to the previous behavior starting from the user viewpoint. Unconfirmed funds allow you to spend from then or consider likely-yours as part of your balance, but here we know, as of mempool state, they are conflicted and likely-but-not-certain not going to confirm. We don't want the user to take decisions based on funds he has not. You can spend from unconfirmed, right ?
Knowing there are cases the wallet can't detect unconfirmed transactions becoming conflicted doesn't mean that when we have the information available we shouldn't act on this.
Thanks for
feature_notifications.py, we will modify to test notifications, didn't know its existence.
I think you can't generally spend from unconfirmed transactions unless the transaction is "trusted" (coming from you), in which case the normal wallet conflict detection should work. So I don't think there's a strong rationale for introducing new behavior and a new transaction state instead of just fixing the bug more simply and restoring previous behavior.
I'm also not sure the change to GetDepthInMainChain is sufficient to handle the new conflicted state. For example in LoadToWallet I see
Lines 907 to 909 in 4d793bc
Is this going to work correctly when m_confirm.hashBlock is null, or does it need to be updated too? If GetDepthInMainChain and LoadToWallet have to be updated to deal with the new state, and these transactions are serialized to wallet files, what happens if the wallets are loaded by older wallet software? Will 0.20 and 0.19 GetDepthInMainChain and LoadToWallet implementations handle the new state correctly without bugs and crashes?
There was a problem hiding this comment.
avoid introducing an entirely new transaction state that wallet code has never had to deal with, where a transaction is conflicting with something not in a specific block.
Ah sorry reading this again, in fact in transactionRemovedFromMempool you may have case of transaction block conflicted but not covered by BlockConnected due to the conflicting transaction not involving us.
Like unconfirmed txA spent by unconfirmed txB, txB paying a wallet address. txA is conflicted by txA' in a block, we won't see it in BlockConnected right now.
So this PR restores UI notifications and extend scope of block conflicted transactions in the lightweight way I think. Detecting them at block connection is not possible without tracking whole chain of unconfirmed transactions AFAICT.
Is this going to work correctly when m_confirm.hashBlock is null, or does it need to be updated too?
Child conflicted transactions is going to inherit m_confirm.hashBlock null from parent.
If GetDepthInMainChain and LoadToWallet have to be updated to deal with the new state, and these transactions are serialized to wallet files, what happens if the wallets are loaded by older wallet software?
It's not a new state, as Confirmation::Status::CONFLICTED" is already there. I think what matters is among CONFLICTEDtransactions is there any code path relying onhashBlockorblock_height, beyond display code, there is MarkConflictedto keep track of the most conflicting code andGetDepthInMainChain` ofc.
Will 0.20 and 0.19 GetDepthInMainChain and LoadToWallet implementations handle the new state correctly without bugs and crashes?
As said previously, it's not a new state, and you already have of course UNCONFIRMED with null hash and zero-height on which current serialization/deserialization logic relies on :
Line 434 in b6a5dc9
Trying to go forward, I think best would be to pass block height and hash in transactionsRemovedFromMempool even if it seems counter-intuitive, for block conflicting transaction we do have a block height/hash.
What do you think ?
There was a problem hiding this comment.
re: #18600 (comment)
It's not a new state
The new state which this writes is:
uint256 hashBlock |
int nIndex |
interpretation |
|---|---|---|
| 0 | -1 | conflicted since #16624, unconfirmed prior |
The existing states written are:
uint256 hashBlock |
int nIndex |
interpretation |
|---|---|---|
| 0 | 0 | unconfirmed |
| >1 | >=0 | confirmed |
| >1 | -1 | conflicted since #7105, unconfirmed prior |
| 1 | -1 | abandoned since #7312, unconfirmed prior |
Never written and still unused states are:
uint256 hashBlock |
int nIndex |
interpretation |
|---|---|---|
| 0 | not 0 or -1 | unconfirmed |
| 1 | not -1 | abandoned between #7312 and #16624, unconfirmed before and after |
| >1 | <-1 | confirmed |
Maybe it's good to add a new state, or maybe it's bad, but it doesn't seem like enough thought has gone into it the decision so far. Looking at CWalletTx::GetDepthInMainChain, CWallet::ComputeTimeSmart, CWallet::MarkConflicted all of these methods seem broken by the new state. If I am reading correctly, GetDepthInMainChain will return large bogus negative height values, MarkConflicted 'more conflicted' check will start overwriting valid conflicted blocks with null conflict blocks, ComputeTimeSmart will logging spurious found in block not in index errors. Am I misreading the code in seeing these behaviors? Or do you see the same behaviors, but think side effects are minimal? Do you think existing 0.17, 0.18, 0.19 wallet code handles transactions in the new state safely?
Trying to go forward, I think best would be to pass block height and hash in
transactionsRemovedFromMempool
This does seem like a safer way forward, and probably a good approach. Only caveat is the usual caveat with changing the wallet conflicted transaction heuristic: that if we start marking transactions conflicted instead of unconfirmed, the wallet can fail to notice them becoming unconfirmed again if they become unconflicted later. This should be less likely when the conflict is coming from a block transaction instead of a mempool transaction, but not certain. I think if we are going to change the heuristic, we can't just think about it abstractly. We should be looking at specific cases and trying to figure out if users are helped or hurt by the change in each case.
There was a problem hiding this comment.
You're right this approach by introducing a new state is broken. I think I've been confused by trying to solve #18325 and at same time try to harden conflict detection on non-connected wallet transaction.
I think the safe approach is to pass block height and hash in transactionsRemovedFromMempool but in another PR to reason on its own.
Note: IIRC we don't have undo conflict logic, that was the whole intent of #17543.
b34318d to
e0b7199
Compare
e0b7199 to
b3c9467
Compare
…nflicted Client may be interested in updating status of notified transaction without tracking a whole chain of anterior transactions conflicted, independently of the reason (mempool or block). This is only used in next commit.
b3c9467 to
37f4b97
Compare
Extend conflict tracking for transaction of interest (either spending from us or paying to us) not directly connected to a previous wallet transaction and as such not covered under blockConnected. Restore UI notifications for block conflicted transactions broke inadvertently by 7e89994. Block conflicted transactions may see their CWalletTx::Confirmation status updated twice, once by BlockConnected and transactionRemovedFromMempool, overlap is necessary as first trigger may be due to the conflicting transaction included in a block at AddToWalletIfInvolvingMe and second one to mempool removal on its own sake.
Previous commit was extending conflicts tracking to ones triggered from mempool. These do not informed about txid of conflicting transaction, so we need a way to learn about conflict status of a wallet transaction. New field is fulfilled for listtransactions, listsinceblock, gettransaction.
Non-connected is defined as a transactions for which parent or anterior transactions are not tracked by wallet. This test extension covers non-connected wallet transaction conflicted by a block connection.
37f4b97 to
e9ffced
Compare
|
Rebased with few comments addressed. I hear you on this #18600 (comment) @ryanofsky, I may have to pass down block height/hash through mempool code but at least that would make reasoning the same than it is today for I also know you're not a great fan of these states, IMO I find pretty nice to have well-defined states for wallet transactions and grepp'ing for I also dropped mempool replacement considered as conflict in wallet code as it was controversial. |
I need to catch up on the newest comments and changes, but I think it'd be great to improve state tracking in the ways you're suggesting, and I think #16624 was a big code improvement. I was just expecting this PR to be smaller and to just restore previous notification behavior, fixing the regression instead of doing bigger things in ways that looked like they could affect backwards compatibility. In the end since the wallet has incomplete information, there probably isn't going to be an obviously ideal way to track states, and we'll need to consider a lot of individual use cases. I think the main goal should be eliminating cases where the wallet displays misleading information or forces users into workarounds like calling |
|
Tests fail |
Add test coverage for conflicted wallet transaction notifications so we improve current behavior and avoid future regressions bitcoin#9240 - accidental break bitcoin#9479 - bug report bitcoin#9371 - fix bitcoin#16624 - accidental break bitcoin#18325 - bug report bitcoin#18600 - potential fix
There was a problem hiding this comment.
Thanks for your patience ariard! I like the direction you're taking this code even though it's lot of work to think through and even though it runs against my bias for smaller more incremental changes.
As of e9ffced, this PR seems to be doing a few different things:
- Sending notifications for wallet transactions removed from the mempool due to conflicts with a new block. These were previously sent before #16624 but accidentally broken.
- Sending notifications for wallet transactions removed from the mempool due to conflicts with new mempool transactions. These weren't sent before but were considered in #9371.
- Marking wallet transactions removed from the mempool due to conflicts with a new block as conflicted, without recordin 10000 g the conflicting block hash. These transactions were not previously marked conflicted even before 16624, only marked as unconfirmed.
- Adding a new "conflicted" field in
WalletTxToJSON.
Changes (1) and (2) above both seem great and I think it'd be great to have them implemented in this PR without complications of (3) and (4). Or alternately (3) and (4) could be pursued here in this PR and (1) and (2) could be done separately. I wrote some new tests in #18878 that add previously missing test coverage for wallet conflict notifications so (1) and (2) can be implemented without needing (3) and (4) for test coverage.
My main concern about (3) is backwards compatibility with previous releases, and potential bugs in current wallet code that can't handle the conflicted with 0-blockhash state. I am not very concerned about (4), but I think it adds complications without being directly related to the other changes here. I am a little concerned though that the new "conflicted" field in (4) might be misinterpreted externally to be a reliable indicator of whether a wallet transaction is conflicted instead of just a heuristic
| LOCK(cs_wallet); | ||
| auto it = mapWallet.find(ptx->GetHash()); | ||
| if (it != mapWallet.end()) { | ||
| it->second.fInMempool = false; | ||
| } | ||
| if (fIsConflicted) { | ||
| CWalletTx::Confirmation confirm(CWalletTx::Status::CONFLICTED, /* block_height */ 0, /* block_hash */ {}, /* nIndex */ 0); |
There was a problem hiding this comment.
re: #18600 (comment)
It's not a new state
The new state which this writes is:
uint256 hashBlock |
int nIndex |
interpretation |
|---|---|---|
| 0 | -1 | conflicted since #16624, unconfirmed prior |
The existing states written are:
uint256 hashBlock |
int nIndex |
interpretation |
|---|---|---|
| 0 | 0 | unconfirmed |
| >1 | >=0 | confirmed |
| >1 | -1 | conflicted since #7105, unconfirmed prior |
| 1 | -1 | abandoned since #7312, unconfirmed prior |
Never written and still unused states are:
uint256 hashBlock |
int nIndex |
interpretation |
|---|---|---|
| 0 | not 0 or -1 | unconfirmed |
| 1 | not -1 | abandoned between #7312 and #16624, unconfirmed before and after |
| >1 | <-1 | confirmed |
Maybe it's good to add a new state, or maybe it's bad, but it doesn't seem like enough thought has gone into it the decision so far. Looking at CWalletTx::GetDepthInMainChain, CWallet::ComputeTimeSmart, CWallet::MarkConflicted all of these methods seem broken by the new state. If I am reading correctly, GetDepthInMainChain will return large bogus negative height values, MarkConflicted 'more conflicted' check will start overwriting valid conflicted blocks with null conflict blocks, ComputeTimeSmart will logging spurious found in block not in index errors. Am I misreading the code in seeing these behaviors? Or do you see the same behaviors, but think side effects are minimal? Do you think existing 0.17, 0.18, 0.19 wallet code handles transactions in the new state safely?
Trying to go forward, I think best would be to pass block height and hash in
transactionsRemovedFromMempool
This does seem like a safer way forward, and probably a good approach. Only caveat is the usual caveat with changing the wallet conflicted transaction heuristic: that if we start marking transactions conflicted instead of unconfirmed, the wallet can fail to notice them becoming unconfirmed again if they become unconflicted later. This should be less likely when the conflict is coming from a block transaction instead of a mempool transaction, but not certain. I think if we are going to change the heuristic, we can't just think about it abstractly. We should be looking at specific cases and trying to figure out if users are helped or hurt by the change in each case.
This fix is a based on the fix by Antoine Riard <ariard@student.42.fr> in bitcoin#18600. Unlike that PR, which implements some new behavior, this just restores previous wallet notification and status behavior for transactions removed from the mempool because they conflict with transactions in a block. The behavior was accidentally changed in two `CWallet::BlockConnected` updates: a31be09 and 7e89994 from bitcoin#16624, causing issue bitcoin#18325. The change here could be improved and replaced with a more comprehensive cleanup, so it includes a detailed comment explaining future considerations. Fixes bitcoin#18325 Co-authored-by: Antoine Riard <ariard@student.42.fr>
…otifications 7eaf86d trivial: Suggested cleanups to surrounding code (Russell Yanofsky) b604c5c wallet: Minimal fix to restore conflicted transaction notifications (Russell Yanofsky) Pull request description: This fix is a based on the fix by Antoine Riard (ariard) in #18600. Unlike that PR, which implements some new behavior, this just restores previous wallet notification and status behavior for transactions removed from the mempool because they conflict with transactions in a block. The behavior was accidentally changed in two `CWallet::BlockConnected` updates: a31be09 and 7e89994 from #16624, causing issue #18325. The change here could be improved and replaced with a more comprehensive cleanup, so it includes a detailed comment explaining future considerations. Fixes #18325 Co-authored-by: Antoine Riard (ariard) ACKs for top commit: jonatack: Re-ACK 7eaf86d ariard: ACK 7eaf86d, reviewed, built and ran tests. MarcoFalke: ACK 7eaf86d 🍡 Tree-SHA512: 9a1efe975969bb522a9dd73c41064a9348887cb67883cd92c6571fd2df4321b9f4568363891abdaae14a3b9b168ef8142e95c373fc04677e46289b251fb84689
|
🐙 This pull request conflicts with the target branch and needs rebase. |
…ction notifications 7eaf86d trivial: Suggested cleanups to surrounding code (Russell Yanofsky) b604c5c wallet: Minimal fix to restore conflicted transaction notifications (Russell Yanofsky) Pull request description: This fix is a based on the fix by Antoine Riard (ariard) in bitcoin#18600. Unlike that PR, which implements some new behavior, this just restores previous wallet notification and status behavior for transactions removed from the mempool because they conflict with transactions in a block. The behavior was accidentally changed in two `CWallet::BlockConnected` updates: a31be09 and 7e89994 from bitcoin#16624, causing issue bitcoin#18325. The change here could be improved and replaced with a more comprehensive cleanup, so it includes a detailed comment explaining future considerations. Fixes bitcoin#18325 Co-authored-by: Antoine Riard (ariard) ACKs for top commit: jonatack: Re-ACK 7eaf86d ariard: ACK 7eaf86d, reviewed, built and ran tests. MarcoFalke: ACK 7eaf86d 🍡 Tree-SHA512: 9a1efe975969bb522a9dd73c41064a9348887cb67883cd92c6571fd2df4321b9f4568363891abdaae14a3b9b168ef8142e95c373fc04677e46289b251fb84689
This fix is a based on the fix by Antoine Riard <ariard@student.42.fr> in bitcoin#18600. Unlike that PR, which implements some new behavior, this just restores previous wallet notification and status behavior for transactions removed from the mempool because they conflict with transactions in a block. The behavior was accidentally changed in two `CWallet::BlockConnected` updates: a31be09 and 7e89994 from bitcoin#16624, causing issue bitcoin#18325. The change here could be improved and replaced with a more comprehensive cleanup, so it includes a detailed comment explaining future considerations. Fixes bitcoin#18325 Co-authored-by: Antoine Riard <ariard@student.42.fr>
This fix is a based on the fix by Antoine Riard <ariard@student.42.fr> in bitcoin#18600. Unlike that PR, which implements some new behavior, this just restores previous wallet notification and status behavior for transactions removed from the mempool because they conflict with transactions in a block. The behavior was accidentally changed in two `CWallet::BlockConnected` updates: a31be09 and 7e89994 from bitcoin#16624, causing issue bitcoin#18325. The change here could be improved and replaced with a more comprehensive cleanup, so it includes a detailed comment explaining future considerations. Fixes bitcoin#18325 Co-authored-by: Antoine Riard <ariard@student.42.fr> Github-Pull: bitcoin#18982 Rebased-From: b604c5c
This fix is a based on the fix by Antoine Riard <ariard@student.42.fr> in bitcoin#18600. Unlike that PR, which implements some new behavior, this just restores previous wallet notification and status behavior for transactions removed from the mempool because they conflict with transactions in a block. The behavior was accidentally changed in two `CWallet::BlockConnected` updates: a31be09 and 7e89994 from bitcoin#16624, causing issue bitcoin#18325. The change here could be improved and replaced with a more comprehensive cleanup, so it includes a detailed comment explaining future considerations. Fixes bitcoin#18325 Co-authored-by: Antoine Riard <ariard@student.42.fr> Github-Pull: bitcoin#18982 Rebased-From: b604c5c
…ction notifications 7eaf86d trivial: Suggested cleanups to surrounding code (Russell Yanofsky) b604c5c wallet: Minimal fix to restore conflicted transaction notifications (Russell Yanofsky) Pull request description: This fix is a based on the fix by Antoine Riard (ariard) in bitcoin#18600. Unlike that PR, which implements some new behavior, this just restores previous wallet notification and status behavior for transactions removed from the mempool because they conflict with transactions in a block. The behavior was accidentally changed in two `CWallet::BlockConnected` updates: a31be09 and 7e89994 from bitcoin#16624, causing issue bitcoin#18325. The change here could be improved and replaced with a more comprehensive cleanup, so it includes a detailed comment explaining future considerations. Fixes bitcoin#18325 Co-authored-by: Antoine Riard (ariard) ACKs for top commit: jonatack: Re-ACK 7eaf86d ariard: ACK 7eaf86d, reviewed, built and ran tests. MarcoFalke: ACK 7eaf86d dango Tree-SHA512: 9a1efe975969bb522a9dd73c41064a9348887cb67883cd92c6571fd2df4321b9f4568363891abdaae14a3b9b168ef8142e95c373fc04677e46289b251fb84689
…ction notifications 7eaf86d trivial: Suggested cleanups to surrounding code (Russell Yanofsky) b604c5c wallet: Minimal fix to restore conflicted transaction notifications (Russell Yanofsky) Pull request description: This fix is a based on the fix by Antoine Riard (ariard) in bitcoin#18600. Unlike that PR, which implements some new behavior, this just restores previous wallet notification and status behavior for transactions removed from the mempool because they conflict with transactions in a block. The behavior was accidentally changed in two `CWallet::BlockConnected` updates: a31be09 and 7e89994 from bitcoin#16624, causing issue bitcoin#18325. The change here could be improved and replaced with a more comprehensive cleanup, so it includes a detailed comment explaining future considerations. Fixes bitcoin#18325 Co-authored-by: Antoine Riard (ariard) ACKs for top commit: jonatack: Re-ACK 7eaf86d ariard: ACK 7eaf86d, reviewed, built and ran tests. MarcoFalke: ACK 7eaf86d dango Tree-SHA512: 9a1efe975969bb522a9dd73c41064a9348887cb67883cd92c6571fd2df4321b9f4568363891abdaae14a3b9b168ef8142e95c373fc04677e46289b251fb84689
This fix is a based on the fix by Antoine Riard <ariard@student.42.fr> in bitcoin#18600. Unlike that PR, which implements some new behavior, this just restores previous wallet notification and status behavior for transactions removed from the mempool because they conflict with transactions in a block. The behavior was accidentally changed in two `CWallet::BlockConnected` updates: a31be09 and 7e89994 from bitcoin#16624, causing issue bitcoin#18325. The change here could be improved and replaced with a more comprehensive cleanup, so it includes a detailed comment explaining future considerations. Fixes bitcoin#18325 Co-authored-by: Antoine Riard <ariard@student.42.fr>
This fix is a based on the fix by Antoine Riard <ariard@student.42.fr> in bitcoin/bitcoin#18600. Unlike that PR, which implements some new behavior, this just restores previous wallet notification and status behavior for transactions removed from the mempool because they conflict with transactions in a block. The behavior was accidentally changed in two `CWallet::BlockConnected` updates: a31be09 and 7e89994 from bitcoin/bitcoin#16624, causing issue bitcoin/bitcoin#18325. The change here could be improved and replaced with a more comprehensive cleanup, so it includes a detailed comment explaining future considerations. Fixes #18325 Co-authored-by: Antoine Riard <ariard@student.42.fr> Github-Pull: #18982 Rebased-From: b604c5c
This fix is a based on the fix by Antoine Riard <ariard@student.42.fr> in bitcoin#18600. Unlike that PR, which implements some new behavior, this just restores previous wallet notification and status behavior for transactions removed from the mempool because they conflict with transactions in a block. The behavior was accidentally changed in two `CWallet::BlockConnected` updates: a31be09 and 7e89994 from bitcoin#16624, causing issue bitcoin#18325. The change here could be improved and replaced with a more comprehensive cleanup, so it includes a detailed comment explaining future considerations. Fixes bitcoin#18325 Co-authored-by: Antoine Riard <ariard@student.42.fr> Github-Pull: bitcoin#18982 Rebased-From: b604c5c
Add test coverage for conflicted wallet transaction notifications so we improve current behavior and avoid future regressions bitcoin/bitcoin#9240 - accidental break bitcoin/bitcoin#9479 - bug report bitcoin/bitcoin#9371 - fix bitcoin/bitcoin#16624 - accidental break bitcoin/bitcoin#18325 - bug report bitcoin/bitcoin#18600 - potential fix
This fix is a based on the fix by Antoine Riard <ariard@student.42.fr> in bitcoin/bitcoin#18600. Unlike that PR, which implements some new behavior, this just restores previous wallet notification and status behavior for transactions removed from the mempool because they conflict with transactions in a block. The behavior was accidentally changed in two `CWallet::BlockConnected` updates: a31be09bfd77eed497a8e251d31358e16e2f2eb1 and 7e89994133725125dddbfa8d45484e3b9ed51c6e from bitcoin/bitcoin#16624, causing issue bitcoin/bitcoin#18325. The change here could be improved and replaced with a more comprehensive cleanup, so it includes a detailed comment explaining future considerations. Fixes #18325 Co-authored-by: Antoine Riard <ariard@student.42.fr>
|
Since this PR has merge conflicts and the underlying code has changed a lot, I'm marking it closed and up for grabs. This way if it's updated it can be a new PR with less comment history, and the comments in this PR can stay in line with diffs. The main change implemented by this PR is treating wallet transactions that conflict with newly connected blocks as conflicted. There is more information about this idea in idea-more-conflicts, and it requires some more work to implement correctly. To avoid the state representation problems described #18600 (comment), it needs some updates to node code to pass along the conflicting block hash with conflicted removal notifications. It also needs extra wallet code to undo conflicts during reorgs. A related idea idea-mempool-conflicted could be a way of treating conflicts similarly but requiring fewer changes. The PR's current approach of pretending block-connected conflicts are mempool conflicts instead of block conflicts could be kept, with serialization tweaks to just treat the mempool-conflicted state as temporary and not write it to the database. The other changes in this PR like adding a "conflicted" RPC field and new test coverage (see #18600 (review)) could probably also be made into standalone PRs that could be merged pretty quickly. |
Add test coverage for conflicted wallet transaction notifications so we improve current behavior and avoid future regressions bitcoin/bitcoin#9240 - accidental break bitcoin/bitcoin#9479 - bug report bitcoin/bitcoin#9371 - fix bitcoin/bitcoin#16624 - accidental break bitcoin/bitcoin#18325 - bug report bitcoin/bitcoin#18600 - potential fix Github-Pull: #18878 Rebased-From: f963a68
Summary: > This fix is a based on the fix by Antoine Riard <ariard@student.42.fr> in > bitcoin/bitcoin#18600. > > Unlike that PR, which implements some new behavior, this just F438 restores previous > wallet notification and status behavior for transactions removed from the > mempool because they conflict with transactions in a block. The behavior was > accidentally changed in two `CWallet::BlockConnected` updates: > a31be09bfd77eed497a8e251d31358e16e2f2eb1 and > 7e89994133725125dddbfa8d45484e3b9ed51c6e from > bitcoin/bitcoin#16624, causing issue > bitcoin/bitcoin#18325. > > The change here could be improved and replaced with a more comprehensive > cleanup, so it includes a detailed comment explaining future considerations. > > Co-authored-by: Antoine Riard <ariard@student.42.fr> This is a backport of [[bitcoin/bitcoin#18982 | core#18982]] [1/2] bitcoin/bitcoin@b604c5c Backport note: The test is already included in D9972, because it seemed to work without this fix. But now we have seen intermittent CI failures. Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: majcosta Differential Revision: https://reviews.bitcoinabc.org/D9962
…tions f963a68 test: Add test for conflicted wallet tx notifications (Russell Yanofsky) Pull request description: Add test coverage for conflicted wallet transaction notifications so we can improve current behavior and avoid future regressions bitcoin#9240 - accidental break bitcoin#9479 - bug report bitcoin#9371 - fix bitcoin#16624 - accidental break bitcoin#18325 - bug report bitcoin#18600 - potential fix ACKs for top commit: laanwj: ACK f963a68 jonatack: re-ACK f963a68 MarcoFalke: ACK f963a68 Tree-SHA512: d3a7952a2d3dc2ff0800ef857575ea4ef9759c0917d58a7fc91e2db0ca3cc3baf0dd0cf9af61683f691e5fefb11afe8120bb5810c7037ed9ecedee385dd4aa07
…tions f963a68 test: Add test for conflicted wallet tx notifications (Russell Yanofsky) Pull request description: Add test coverage for conflicted wallet transaction notifications so we can improve current behavior and avoid future regressions bitcoin#9240 - accidental break bitcoin#9479 - bug report bitcoin#9371 - fix bitcoin#16624 - accidental break bitcoin#18325 - bug report bitcoin#18600 - potential fix ACKs for top commit: laanwj: ACK f963a68 jonatack: re-ACK f963a68 MarcoFalke: ACK f963a68 Tree-SHA512: d3a7952a2d3dc2ff0800ef857575ea4ef9759c0917d58a7fc91e2db0ca3cc3baf0dd0cf9af61683f691e5fefb11afe8120bb5810c7037ed9ecedee385dd4aa07
|
Dropped up for grabs here, see #27307. |
UI notifications and the -walletnotify process should be triggered for wallet transactions that are conflicted out of the mempool by a newly connected block.
Unfortunately, it has been broken by 7e89994 by removing the call to
SyncTransaction()for block-conflicted transactions inCWallet::BlockConnected. This PR restores conflict tracking but instead relies onTransactionRemovedFromMempool.Doing so allows for wider conflict detection, i.e non-connected wallet transactions. Transaction A may not involves wallet but may be spent by a transaction B paying back to a wallet address. Double-spend of transaction A won't trigger a conflict for transaction B as A isn't tracked by wallet (
mapTxSpends).This PR also extends conflicts detection to mempool replacement (
MemPoolRemovalReason::REPLACED), as for an end-user, consequences are likely to be interpreted the same, conflicted funds aren't available. Distinction of reasons (block-connected or mempool-replacement) should stay clear for us.Conflicts detection stays a best-effort, as a mempool replacement may happen while wallet is shutdown, and mempool isn't replayed at wallet rescan, conflict can't be see.
Fixes #18325