[wallet] Track conflicted transactions removed from mempool and fix UI notifications

ariard commented at 11:44 pm on April 11, 2020: member

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 in CWallet::BlockConnected. This PR restores conflict tracking but instead relies on TransactionRemovedFromMempool.

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

fanquake added the label Wallet on Apr 11, 2020

fanquake added the label Needs rebase on Apr 11, 2020

ariard force-pushed on Apr 11, 2020

fanquake removed the label Needs rebase on Apr 11, 2020

fanquake requested review from jnewbery on Apr 11, 2020

DrahtBot commented at 4:13 am on April 12, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#18982 (wallet: Minimal fix to restore conflicted transaction notifications by ryanofsky)
#18354 (Use shared pointers only in validation interface by bvbfan)
#17443 (Drop checkFinalTx and use Median Time Past to check finality of wallet transactions by ariard)
#15719 (Wallet passive startup by ryanofsky)

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.

in src/interfaces/chain.cpp:161 in 9eb9af6c56 outdated

157@@ -158,7 +158,7 @@ class NotificationsProxy : public CValidationInterface
158     {
159         m_notifications->transactionAddedToMempool(tx);
160     }
161-    void TransactionRemovedFromMempool(const CTransactionRef& tx) override
162+    void TransactionRemovedFromMempool(const CTransactionRef& tx, bool fIsConflicted) override

ryanofsky commented at 2:37 pm on April 13, 2020:

In commit “[validationinterface] Extend TransactionRemovedFromMempool with fIsConflicted” (9eb9af6c56f52628cca4cc5241c0f88adb9f689d)

Should just call this argument conflicted or is_conflicted

MarcoFalke commented at 3:50 pm on April 13, 2020:

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.

ariard commented at 7:55 am on April 14, 2020:

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.

MarcoFalke commented at 11:27 am on April 14, 2020:

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

ryanofsky commented at 0:42 am on April 15, 2020:

re: #18600 (review)

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 ?

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 declare enum class MemPoolRemovalReason; for now, and longer term if the interface stabilizes the actual enum definition could move there.

ariard commented at 0:50 am on April 22, 2020:

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 ?

ryanofsky commented at 3:22 pm on May 5, 2020:

re: #18600 (review)

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 (review) 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

ariard commented at 6:46 am on May 20, 2020:

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.

in src/txmempool.cpp:413 in 9eb9af6c56 outdated

409@@ -410,7 +410,8 @@ void CTxMemPool::removeUnchecked(txiter it, MemPoolRemovalReason reason)
410         // for any reason except being included in a block. Clients interested
411         // in transactions included in blocks can subscribe to the BlockConnected
412         // notification.
413-        GetMainSignals().TransactionRemovedFromMempool(it->GetSharedTx());
414+        GetMainSignals().TransactionRemovedFromMempool(it->GetSharedTx(), reason == MemPoolRemovalReason::CONFLICT

ryanofsky commented at 2:46 pm on April 13, 2020:

In commit “[validationinterface] Extend TransactionRemovedFromMempool with fIsConflicted” (9eb9af6c56f52628cca4cc5241c0f88adb9f689d)

I’d think it’d be more straightforward to just pass along the reason instead of having the mempool interpret it on behalf of the wallet. But this is just a guess, and I think since @jnewbery proposed this, you both probably know the tradeoffs between bool conflicted and MemPoolRemovalReason reason better than me and this is probably fine

ariard commented at 8:07 am on April 14, 2020:

See above, on the practical-side I wanted to avoid linking mempool code in wallet. But we can overcome this, so real question, above fixing behavior, is if we pass EXPIRY or SIZELIMIT to the wallet, does it going to adapt its broadcast on this or take any other action ?

Right now I think the wallet doesn’t have any code to do this, so I think the discussion is worthwhile but beyond this PR ?

Actually, digging into this, MemPoolRemovalReason purpose seems weak right now because we only use it in removeUnchecked to decide if we should trigger TransactionsRemovedFromMempool ?

ryanofsky commented at 3:23 pm on May 5, 2020:

re: #18600 (review)

Actually, digging into this, MemPoolRemovalReason purpose seems weak right now because we only use it in removeUnchecked to decide if we should trigger TransactionsRemovedFromMempool ?

This thread was continued #18600 (review) where Marco made a similar suggestion. But to respond to “purpose seems weak”, the suggestion wasn’t based on what wallet conflict tracking code is currently doing. Wallet conflict tracking has changed before and will likely change in the future. The suggestion to use the existing reason code was avoid to being inconsistent and confusing and avoid implementing wallet behavior outside of the wallet

in src/wallet/wallet.cpp:1118 in 497c502055 outdated

1111     auto it = mapWallet.find(ptx->GetHash());
1112     if (it != mapWallet.end()) {
1113         it->second.fInMempool = false;
1114     }
1115+    if (fIsConflicted) {
1116+        CWalletTx::Confirmation confirm(CWalletTx::Status::CONFLICTED, /* block_height */ 0, /* block_hash */ {}, /* nIndex */ 0);

ryanofsky commented at 3:39 pm on April 13, 2020:

In commit “[rpcwallet] Add conflicted as transaction description field” (1af561a8f2bc93ac029b7304fb6da12e6094b860)

I think this should probably be Status::UNCONFIRMED rather than Status::CONFLICTED. That should restore the original behavior before #16624 (a31be09bfd77eed497a8e251d31358e16e2f2eb1 + 7e89994133725125dddbfa8d45484e3b9ed51c6e) 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

ariard commented at 8:23 am on April 14, 2020:

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.

ryanofsky commented at 0:43 am on April 15, 2020:

re: #18600 (review)

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

https://github.com/bitcoin/bitcoin/blob/4d793bcfe81479d1bf34805c3960c4611eb8fc9b/src/wallet/wallet.cpp#L907-L909

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?

ariard commented at 0:44 am on April 22, 2020:

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 : https://github.com/bitcoin/bitcoin/blob/b6a5dc90bfd4640cf9f914e59bf8e21cd265b51e/src/wallet/wallet.h#L434

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 ?

ryanofsky commented at 3:24 pm on May 5, 2020:

re: #18600 (review)

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.

ariard commented at 7:03 am on May 20, 2020:

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.

ryanofsky commented at 3:45 pm on April 13, 2020: member

Nice to see progress on this!

Started review (will update list below with progress).

9eb9af6c56f52628cca4cc5241c0f88adb9f689d [validationinterface] Extend TransactionRemovedFromMempool with fIsConflicted (1/4)
497c50205534aa32931a048d02a3a2004460fe64 [wallet] Track transactions removed from mempool due to conflicts (2/4)
1af561a8f2bc93ac029b7304fb6da12e6094b860 [rpcwallet] Add conflicted as transaction description field (3/4)
b34318d4cc27d61d2b9cc6a97c9584eb0dada6e4 [functional] Test conflict on non-connected wallet transactions (4/4)

in test/functional/wallet_txn_doublespend.py:195 in b34318d4cc outdated

189+        conflicting_tx= self.nodes[1].signrawtransactionwithwallet(self.nodes[1].createrawtransaction(inputs_conflicting_tx, outputs_conflicting_tx))
190+        conflicting_txid = self.nodes[1].sendrawtransaction(conflicting_tx["hex"])
191+        self.nodes[1].generate(1)
192+        self.sync_blocks([self.nodes[0], self.nodes[1]])
193+
194+        # Verify node0 sees children as conflicted

MarcoFalke commented at 3:59 pm on April 13, 2020:

Some comments might be better as log statements, to aid debugability. See https://github.com/bitcoin/bitcoin/tree/master/test/functional#general-test-writing-advice

0        self.log.info('Verify node0 sees children as conflicted')

ariard commented at 8:32 am on April 14, 2020:

Can I turn every new comment as a log ? That would my personal flavor but that’s hard to judge if it suits everyone. In practice, this advice maybe a bit loosely, I expect anyone to tweak tests as wish for debugging, that’s easy and you may not want to observe same thing for everyone.

MarcoFalke commented at 11:29 am on April 14, 2020:

I mean mostly for the comments that start a new “section” or “paragraph” or subtest/test case. In the failing logs you could then easily identify when the new test section begins.

jonatack commented at 4:40 pm on May 22, 2020:

I mean mostly for the comments that start a new “section” or “paragraph” or subtest/test case. In the failing logs you could then easily identify when the new test section begins.

Yes! and logging like this also makes it clear where the slower tests are to possibly speed them up.

ariard force-pushed on Apr 14, 2020

DrahtBot added the label Needs rebase on Apr 14, 2020

MarcoFalke added this to the milestone 0.20.0 on Apr 16, 2020

ariard force-pushed on Apr 22, 2020

[validationinterface] Extend TransactionRemovedFromMempool with fIsConflicted

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.

8535460000

ariard force-pushed on Apr 22, 2020

[wallet] Track transactions removed from mempool due to conflicts

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.

bdf40f33ea

[rpcwallet] Add `conflicted` as transaction description field

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.

0e5f7c0658

[functional] Test conflict on non-connected wallet transactions

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.

e9ffced8be

ariard force-pushed on Apr 22, 2020

ariard commented at 1:05 am on April 22, 2020: member

Rebased with few comments addressed.

I hear you on this #18600 (review) @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 CONFLICTED state.

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 is*/set* to know state transitions, but I agree too we may refine them to only UNCONFIRMED/CONFIRMED, conflicting is a per-input problem stricto sensu but I think that’s outside PR scope ?

I also dropped mempool replacement considered as conflict in wallet code as it was controversial.

DrahtBot removed the label Needs rebase on Apr 22, 2020

ryanofsky commented at 4:04 am on April 22, 2020: member

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 is*/set* to know state transitions, but I agree too we may refine them to only UNCONFIRMED/CONFIRMED, conflicting is a per-input problem stricto sensu but I think that’s outside PR scope ?

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 abandontransaction. Seeing specific test cases and examples that are improved will really help motivate future changes.

MarcoFalke commented at 10:59 pm on April 29, 2020: member

Tests fail

ryanofsky referenced this in commit 2c1c162cd5 on May 5, 2020

ryanofsky commented at 3:31 pm on May 5, 2020: member

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 e9ffced8bebe7451d5e8d97b5e88fbedb7675cc6, 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 recording 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

ryanofsky referenced this in commit f963a68051 on May 13, 2020

laanwj referenced this in commit 99c03728c0 on May 13, 2020

MarcoFalke commented at 2:31 pm on May 13, 2020: member

Needs rebase due to conflict with #18878

ryanofsky commented at 7:23 pm on May 13, 2020: member

Needs rebase due to conflict with #18878

Changes should be minor. Should just need to update lines like expect_wallet_notify([bump2]) to expect_wallet_notify([bump2, tx2]) for new transaction notifications

sidhujag referenced this in commit a84dd1fa2e on May 14, 2020

fanquake referenced this in commit 9c193e4d0a on May 14, 2020

MarcoFalke added the label Waiting for author on May 14, 2020

MarcoFalke added the label Needs backport (0.20) on May 14, 2020

fanquake referenced this in commit ed0afe8c1f on May 15, 2020

MarcoFalke removed this from the milestone 0.20.0 on May 15, 2020

MarcoFalke added this to the milestone 0.20.1 on May 15, 2020

MarcoFalke commented at 12:04 pm on May 15, 2020: member

Moved milestone as per #18325 (comment)

ryanofsky referenced this in commit 261ccf730c on May 15, 2020

ryanofsky referenced this in commit 58ec22284e on May 15, 2020

ryanofsky referenced this in commit 90118ec54f on May 15, 2020

ariard commented at 7:04 am on May 20, 2020: member

@ryanofsky Finally getting back to this,

As of e9ffced, I think I removed 2) after this discussion #18600 (review).

With regards to (4) it’s a direct dependency on (3), and used only to signal a 0-blockhash conflict to user. So dropping (3) would also drop (4).

Looking on #18982

ryanofsky commented at 11:14 pm on May 20, 2020: member

re: ariard #18600 (comment) re: ryanofsky #18600#pullrequestreview-405898189

As of e9ffced, I think I removed 2) after this discussion #18600 (comment).

Yes, good point. I didn’t really internalize the changed definition of the conflicted bool (one reason I prefer the enum to the bool!) and I missed this.

I actually think it’d be good to bring back behavior (2). We already have an existing test for this case and I think after #18982 it should be safe and very easy to do as a followup.

I also like your idea in #18600 (comment) of passing down the block hash/height so it is safe to mark transactions that conflict with a specific block as conflicted instead of unconfirmed.

I think changes (3) and (4) are good too, my concerns there were just about compatibility and bugs.

Looking on #18982

Thanks for reviewing that! I starting implementing your suggestions there.

ryanofsky referenced this in commit 8242b093d3 on May 21, 2020

jonatack commented at 4:43 pm on May 22, 2020: member

I actually think it’d be good to bring back behavior (2). We already have an existing test for this case and I think after #18982 it should be safe and very easy to do as a followup.

I also like your idea in #18600 (comment) of passing down the block hash/height so it is safe to mark transactions that conflict with a specific block as conflicted instead of unconfirmed.

I think changes (3) and (4) are good too, my concerns there were just about compatibility and bugs.

Concept ACK. I’ll try to review this when it’s updated. Edit: I second the preference for an enum over a bool in many cases; it’s often clearer and generally more extensible for handling state.

ryanofsky referenced this in commit b604c5c8b5 on May 22, 2020

MarcoFalke referenced this in commit 3657aee2d2 on Jun 2, 2020

DrahtBot added the label Needs rebase on Jun 2, 2020

DrahtBot commented at 10:30 pm on June 2, 2020: member

🐙 This pull request conflicts with the target branch and needs rebase.

MarcoFalke removed the label Needs backport (0.20) on Jun 2, 2020

MarcoFalke removed the label Waiting for author on Jun 2, 2020

MarcoFalke removed this from the milestone 0.20.1 on Jun 2, 2020

sidhujag referenced this in commit 6007f5d865 on Jun 3, 2020

ComputerCraftr referenced this in commit 6178216494 on Jun 5, 2020

fanquake referenced this in commit 654420d6df on Jun 9, 2020

luke-jr referenced this in commit 32d773bfc1 on Jun 9, 2020

ComputerCraftr referenced this in commit 48245eb566 on Jun 10, 2020

ComputerCraftr referenced this in commit 077eb62f7f on Jun 10, 2020

stackman27 referenced this in commit 447f036161 on Jun 26, 2020

metalicjames referenced this in commit 09af10dec5 on Aug 5, 2020

backpacker69 referenced this in commit 2463b02ccf on Sep 8, 2020

Bushstar referenced this in commit 4770dc9d56 on Oct 21, 2020

janus referenced this in commit f690d20f65 on Nov 15, 2020

janus referenced this in commit 6fc95ef892 on Nov 15, 2020

ryanofsky added the label Up for grabs on Jan 11, 2021

ryanofsky commented at 2:27 pm on January 11, 2021: member

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 (review), 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#pullrequestreview-405898189) could probably also be made into standalone PRs that could be merged pretty quickly.

ryanofsky closed this on Jan 11, 2021

backpacker69 referenced this in commit 364a2d33dd on Mar 28, 2021

deadalnix referenced this in commit 250a0d5b25 on Aug 30, 2021

PastaPastaPasta referenced this in commit 83dd5bfee2 on Mar 5, 2022

PastaPastaPasta referenced this in commit 2c6e85966e on Mar 5, 2022

DrahtBot locked this on Aug 16, 2022

fanquake removed the label Up for grabs on Apr 11, 2023

fanquake removed the label Needs rebase on Apr 11, 2023

fanquake commented at 9:55 am on April 11, 2023: member

Dropped up for grabs here, see #27307.

[wallet] Track conflicted transactions removed from mempool and fix UI notifications #18600

Conflicts