wallet: encapsulate transactions state

ariard commented at 9:16 pm on August 15, 2019: member

While working on #15931, I’ve tried to rationalize tx state management to ease integration of block height tracking per-wallet tx. We currently rely on a combination of hashBlock and nIndex with magic value to determine tx confirmation, conflicted or abandoned state. It’s hard to reason and error-prone. To solve that, we encapsulate these fields in a TxConfirmation struct and introduce a TxState member that we update accordingly at block connection/disconnection.

Following jnewbery recommendation, I’ve taken these changes in its own commit, and open a PR to get them first. It would ease review of aforementioned PR, but above all should ease fixing of long-term issues like :

#7315 (but maybe we should abandon abandontransaction or relieve it to only free outpoints not track the transaction as abandoned in itself, need its own discussion)
#8692 where we should cancel conflicted state of transactions chain smoothly
MarkConflicted in LoadToWallet is likely useless if we track conflicts rights at block connection

Main changes of this PR to get right are tx update in AddToWallet and serialization/deserialization logic.

ariard renamed this:
~~wallet : encapsulate trransactions state~~
wallet : encapsulate transactions state
on Aug 15, 2019

DrahtBot added the label RPC/REST/ZMQ on Aug 15, 2019

DrahtBot added the label Tests on Aug 15, 2019

DrahtBot added the label Wallet on Aug 15, 2019

ariard force-pushed on Aug 15, 2019

fanquake removed the label RPC/REST/ZMQ on Aug 15, 2019

ariard commented at 10:14 pm on August 15, 2019: member

Travis is stalling yet another time..

jnewbery commented at 11:17 pm on August 15, 2019: member

Concept ACK

DrahtBot commented at 0:26 am on August 16, 2019: 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:

#15931 (Remove GetDepthInMainChain dependency on locked chain interface by ariard)
#15129 (rpc: Added ability to remove watch only addresses by benthecarman)
#9381 (Remove CWalletTx merging logic from AddToWallet 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/wallet/wallet.h:399 in 7d2a5b39d6 outdated

395@@ -396,7 +396,7 @@ class CWalletTx
396 private:
397     const CWallet* pwallet;
398 
399-  /** Constant used in hashBlock to indicate tx has been abandoned */
400+    /** Constant used in hashBlock to indicate tx has been abandoned, */

Sjors commented at 9:28 am on August 16, 2019:

Nit: why the trailing comma?

in src/wallet/wallet.h:479 in 7d2a5b39d6 outdated

474@@ -477,16 +475,46 @@ class CWalletTx
475         fInMempool = false;
476         nChangeCached = 0;
477         nOrderPos = -1;
478+        tx_state.hashBlock = uint256();
479+        tx_state.nIndex = 0;

Sjors commented at 9:31 am on August 16, 2019:

Why is the default initialization changed to 0?

ryanofsky commented at 4:10 pm on August 16, 2019:

Why is the default initialization changed to 0?

From what I can tell in this PR, the convention mostly adhered to in this PR is to stop using -1 and ABANDON_HASH values at runtime, and restrict them only to the serialization code. This seems like a pretty good convention if it’s followed consistently. I also think it’d be nice to explicitly say what values the block and index values should have for different states in a comment.

ariard commented at 5:44 pm on August 16, 2019:

Yes see comment on top of TxConfirmation, should be improved ?

in src/wallet/wallet.h:513 in 7d2a5b39d6 outdated

514+         * with, but if hashBlock is null it means transaction is abandoned. An nIndex >= 0
515+         * refers to a confirmed transaction (if hashBlock set) or unconfirmed one.
516+         * Older clients interpret nIndex == -1 as unconfirmed for backward
517+         * compatibility (pre-commit 9ac63d6).
518+         */
519+        int nIndex;

Sjors commented at 9:34 am on August 16, 2019:

Can we move this backwards compatibility feature entirely to the serialization code, and then make nIndex a uint (for clarity)?

ryanofsky commented at 4:06 pm on August 16, 2019:

Can we move this backwards compatibility feature entirely to the serialization code, and then make nIndex a uint (for clarity)?

Personally -0 on this. Imo, unsigned types provide no safety and are terrible for everything except bitfields.

ryanofsky commented at 4:08 pm on August 16, 2019:

Just a suggestion, but I’d add = 0 here and = UNCONFIRMED on the line below to simplify constructor and deserialization code.

ariard commented at 5:45 pm on August 16, 2019:

What do you mean here by moving this backwards compatibility feature entirely to the serialization code ? If changes are right this logic should only lived in serialization code now.

Sjors commented at 5:55 pm on August 16, 2019:

See below, it depends on if nIndex has any meaning.

ariard commented at 6:16 pm on August 16, 2019:

done

in src/wallet/wallet.h:508 in 7d2a5b39d6 outdated

509+     */
510+    struct TxConfirmation {
511+        uint256 hashBlock;
512+        /* At serialization/deserialization, an nIndex == -1 means that hashBlock refers to
513+         * the earliest block in the chain we know this or any in-wallet dependency conflicts
514+         * with, but if hashBlock is null it means transaction is abandoned. An nIndex >= 0

Sjors commented at 9:36 am on August 16, 2019:

Mention that nIndex is a block height?

ryanofsky commented at 3:42 pm on August 16, 2019:

if hashBlock is null it means transaction is abandoned

Could you clarify this part of the comment. Doesn’t 1 not 0 mean abandoned?

ariard commented at 5:42 pm on August 16, 2019:

With these changes, but even before, nIndex never referred directly to a block height. When set at -1, it’s interpreted as a magic value meaning the hashBlock is one of the deepest conflicting tx. This PR intents to scope this logic only to serialization/deserialization.

Sjors commented at 5:55 pm on August 16, 2019:

If nIndex doesn’t mean a block height, then why not remove the instance variable altogether? We can still (de)serialize in a backwards compatible way.

ariard commented at 6:10 pm on August 16, 2019:

Currently, AbandonTransaction set nIndex as -1 and this value is kept at serialization/deserialization. I’m not sure if it make sense for older client, because an abandoned transaction can be also confirmed/unconfirmed/conflicted at same time. Nevertheless, I’ve tried to keep same behavior for now, so hashBlock set as ABANDON_HASH is used to avoid deserialization ambiguity with conflicted.

I’ve clarified this comment, the ABANDON_HASH one and also reset abandon txn hashBlock at deserialization. ABANDON_HASH should be scoped at serialization/deserialization.

ariard commented at 7:21 pm on August 16, 2019:

Well I think we need to keep nIndex at least it’s used by RPCs in WalletTxToJSON ? Or getting it out of new TxConfirmation struct ?

Sjors commented at 7:38 pm on August 16, 2019:

I think we should figure out what it means in the RPC, consider deprecating that field, and then having an explicit backwards compatible workaround.

In the backwards compatiblity tests I wrote it seems blockindex is absent for uncofirmned or abandoned transactions. It was 1 for a confirmed transaction.

ryanofsky commented at 10:39 am on August 16, 2019: member

Concept ACK. This overlaps (but is compatible) with #9381

Sjors commented at 10:43 am on August 16, 2019: member

Concept ACK. I added tests for backwards compatibility of abandoned and conflicted wallet transactions to #12134. Those still pass if I apply this PR, which gives some assurance we didn’t break the serialization.

in src/wallet/wallet.h:660 in 0e3212ae85 outdated

659+    void setAbandoned() { tx_state.state = CWalletTx::ABANDONED; }
660+    bool isConflicted() const { return tx_state.state == CWalletTx::CONFLICTED; }
661+    void setConflicted() { tx_state.state = CWalletTx::CONFLICTED; }
662+    bool isUnconfirmed() const { return tx_state.state == CWalletTx::UNCONFIRMED; }
663+    void setUnconfirmed() { tx_state.state = CWalletTx::UNCONFIRMED; }
664+    bool isConfirmed() const { return tx_state.state == CWalletTx::CONFIRMED; }

practicalswift commented at 11:34 am on August 16, 2019:

isConfirmed() does not seem to be tested as the others. Add a test? :-)

ariard commented at 5:48 pm on August 16, 2019:

Removed for now, I’ve added it for further changes were ABANDON state was dual-set with other states, but drop if for now, would have been too big changes at once.

in src/wallet/wallet.h:488 in 0e3212ae85 outdated

488-     * compatibility.
489+
490+    /* New transactions start in the UNCONFIRMED. At BlockConnected,
491+     * they will transition to CONFIRMED. In case of reorg, at BlockDisconnected,
492+     * they rollback to UNCONFIRMED. If we detect a conflicting transaction at
493+     * block connection, we update conflited tx and its dependencies as CONFLICTED.

practicalswift commented at 11:34 am on August 16, 2019:

Should be “conflicted”? :-)

in src/wallet/wallet.cpp:1240 in 7d2a5b39d6 outdated

1237@@ -1249,8 +1238,7 @@ bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef& ptx, const uint256
1238             CWalletTx wtx(this, ptx);
1239 
1240             // Get merkle branch if transaction was found in a block
1241-            if (!block_hash.IsNull())
1242-                wtx.SetMerkleBranch(block_hash, posInBlock);
1243+            wtx.SetMerkleBranch(state, block_hash, posInBlock);

ryanofsky commented at 3:53 pm on August 16, 2019:

Comment above about “if transaction was found in a block” no longer makes sense without the if condition.

Also, it seems like now when an abandoned, conflicted, or confirmed transaction is added to the mempool, and this is called, the transaction will now be marked unconfirmed instead of left in the previous state.

This seems logical, but I’m wondering if it is a change in behavior. It’d be helpful if the PR description or commit message would say explicitly whether any of this changes wallet behavior, and what the changes are if it does.

ariard commented at 7:01 pm on August 16, 2019:

Clarified in commit message that is a change in behavior where at block disconnection, previous tx state is override by UNCONFIRMED one. I think right now, we don’t track at all the fact that tx has been disconnected, and that’s why we don’t undo conflicts, or make it hard to solve them. The only direct consequence of these changes I can think of is a user having to call abandontransaction a 2nd time if block get disconnected and transaction is not back in mempool.

This change would make easier to track conflicts, where if conflicting transaction get UNCONFIRMED, iterate on conflicted outpoints and make them free.

Maybe, I can avoid update right now for UNCONFIRMED state, so a tx is born in this state but never go backward to it, and save implications for a later PR ?

Sjors commented at 10:48 am on August 18, 2019:

I can live with changing it now, but we should drop the comment then.

ryanofsky commented at 4:01 pm on August 19, 2019:

In commit “Encapsulate tx state in a TxConfirmation struct” (8a7d26dc3c69e219a2db8d52980074951aff55a3)

On the surface it seems like improved behavior to not consider a transaction conflicted or abandoned when it’s in the mempool. So just removing or updating the comment above seems good for that case. If this can mark a transaction not abandoned when it’s not actually in the mempool that also seems ok but might be worth noting in a comment.

Ideally this commit might just be a pure refactor, and behavior changes could be left for other commits or PRs, but I think it’s fine to have changes here if they’re explicitly noted.

in src/wallet/wallet.h:551 in 7d2a5b39d6 outdated

547+
548+        if (serializedIndex == -1 && tx_state.hashBlock == ABANDON_HASH) {
549+            tx_state.nIndex = 0;
550+            setAbandoned();
551+        } else if (serializedIndex == -1) {
552+            tx_state.nIndex = -1;

ryanofsky commented at 4:14 pm on August 16, 2019:

Can we set 0 here, or change MarkConflicted to set -1? I think it’d be good to be consistent about what conflicted transactions look like.

ariard commented at 5:51 pm on August 16, 2019:

You’re right that an inconsistency from my part. Corrected.

in src/wallet/wallet.cpp:1418 in 0e3212ae85 outdated

1414@@ -1415,7 +1415,6 @@ void CWallet::BlockConnected(const CBlock& block, const std::vector<CTransaction
1415     // the notification that the conflicted transaction was evicted.
1416 
1417     for (const CTransactionRef& ptx : vtxConflicted) {
1418-        SyncTransaction(ptx, CWalletTx::TxState::CONFLICTED, {} /* block hash */, 0 /* position in block */);

ryanofsky commented at 4:18 pm on August 16, 2019:

In commit “Remove SyncTransaction for conflicted txn in CWallet::BlockConnected” (0e3212ae85c7cf1eb6a8d937ac67d6822f624175)

I don’t think I understand this change. Commit description suggests why it is ok, but I don’t understand what motivates it. Is it just cleanup? Is it maybe a minor bugfix, or needed for a future change?

ariard commented at 7:13 pm on August 16, 2019:

Updated commit message. I think this redundant, as conflicts tagging logic is triggered by connection of conflicting transaction. AFAIK, it’s not a bugfix but a cleanup. And as it was same issue than major commit, I bundled them together.

Sjors commented at 9:43 am on August 18, 2019:

Can you clarify why we keep the first loop at all? Why not call TransactionRemovedFromMempool before SyncTransaction in the second loop?

ariard commented at 2:11 pm on August 19, 2019:

Extended commit message, we keep the loop because set of conflicted txn isn’t same as txn included in a block.

ryanofsky commented at 4:24 pm on August 16, 2019: member

Some questions, but this looks very good and is a welcome change that should make code more readable.

ariard force-pushed on Aug 16, 2019

ariard commented at 7:25 pm on August 16, 2019: member

Thanks all for reviews, updated with comments corrected at 6cc34fc.

ariard force-pushed on Aug 16, 2019

meshcollider commented at 9:47 pm on August 16, 2019: contributor

Concept ACK

in src/wallet/wallet.h:506 in 4ac5d64903 outdated

507+     * where they instead point to block hash and index of the deepest conflicting tx.
508      */
509-    int nIndex;
510+    struct TxConfirmation {
511+        uint256 hashBlock;
512+        /* At serialization/deserialization, an nIndex == -1 means that hashBlock refers to

Sjors commented at 10:16 am on August 18, 2019:

Above this comment: For a CONFIRMED transaction, nIndex is the position inside the block, used in merkle proofs.

This had me confused before; I assumed it referred to a block height.

This serialization comment can be moved to the serialization code, because with this change nIndex is never -1; only serializedIndex is.

ariard commented at 2:19 pm on August 19, 2019:

Moved to the deserialization code!

in src/wallet/wallet.cpp:4622 in 6cc34fc497 outdated

4616@@ -4628,21 +4617,24 @@ CKeyPool::CKeyPool(const CPubKey& vchPubKeyIn, bool internalIn)
4617     m_pre_split = false;
4618 }
4619 
4620-void CWalletTx::SetMerkleBranch(const uint256& block_hash, int posInBlock)
4621+void CWalletTx::SetMerkleBranch(TxState state, const uint256& block_hash, int posInBlock)
4622 {
4623     // Update the tx's hashBlock
4624-    hashBlock = block_hash;
4625+    tx_state.hashBlock = block_hash;

Sjors commented at 10:30 am on August 18, 2019:

TxConfirmation tx_state is now a confusingly named variable.

ariard commented at 2:14 pm on August 19, 2019:

Oh I’ve struggled hard on naming, TxConfirmation is struct with tx state + its values (block hash, position in block). If you have any better names, I’ll take it :)

Sjors commented at 2:37 pm on August 19, 2019:

The variable name tx_state is the confusing part, because it’s not a TxState

ryanofsky commented at 4:19 pm on August 19, 2019:

The variable name tx_state is the confusing part, because it’s not a TxState

I think the current naming is ok, but if it I had to choose, I would probably do something like:

 0class CWalletTx
 1{
 2public:
 3    enum Status { UNCONFIRMED, CONFIRMED, CONFLICTED, ABANDONED };
 4    struct Confirmation {
 5        Status status;
 6        uint256 block_hash
 7        int pos_in_block;
 8    }
 9    ...
10    Confirmation m_confirm;
11    ...
12};

ariard commented at 5:22 pm on August 19, 2019:

Replaced by tx_conf on f9f4926

in src/wallet/wallet.cpp:1374 in 6cc34fc497 outdated

1371@@ -1383,8 +1372,9 @@ void CWallet::MarkConflicted(const uint256& hashBlock, const uint256& hashTx)
1372     }
1373 }
1374 
1375-void CWallet::SyncTransaction(const CTransactionRef& ptx, const uint256& block_hash, int posInBlock, bool update_tx) {
1376-    if (!AddToWalletIfInvolvingMe(ptx, block_hash, posInBlock, update_tx))
1377+void CWallet::SyncTransaction(const CTransactionRef& ptx, CWalletTx::TxState state, const uint256& block_hash, int posInBlock, bool update_tx)

Sjors commented at 11:02 am on August 18, 2019:

posInBlock could be a good rename candidate for nIndex, though probably not worth making the diff bigger.

Sjors approved

Sjors commented at 11:05 am on August 18, 2019: member

ACK 6cc34fc

ariard force-pushed on Aug 19, 2019

ariard commented at 2:21 pm on August 19, 2019: member

Thanks @Sjors for review, pushed some corrections on de4459c

ariard force-pushed on Aug 19, 2019

in src/wallet/wallet.cpp:1129 in 8a7d26dc3c outdated

1136+        if (wtxIn.tx_state.state != wtx.tx_state.state) {
1137+            wtx.tx_state.state = wtxIn.tx_state.state;
1138+            wtx.tx_state.nIndex = wtxIn.tx_state.nIndex;
1139+            wtx.tx_state.hashBlock = wtxIn.tx_state.hashBlock;
1140             fUpdated = true;
1141         }

ryanofsky commented at 4:11 pm on August 19, 2019:

In commit “Encapsulate tx state in a TxConfirmation struct” (8a7d26dc3c69e219a2db8d52980074951aff55a3)

Should we maybe add

0} else {
1    assert(wtx.tx_state.nIndex == wtxIn.tx_state.nIndex);
2    assert(wtx.tx_state.hashBlock = wtxIn.tx_state.hashBlock);
3}

My concern is that the new behavior might be less robust than previous behavior if there is a bug in higher level code that marks a transaction as confirmed in two different blocks without marking the transaction as unconfirmed in between.

ariard commented at 5:27 pm on August 20, 2019:

Done

jnewbery commented at 8:17 pm on August 20, 2019:

(re: #16624 (review))

I don’t like these new asserts. I think they could be triggered under the following scenario:

a wallet tx is confirmed in a block
the wallet is shut down
the block chain re-orgs and the tx is included in a different block
the wallet file is loaded on a sync’ed node

All this code is trying to do is update an existing wallet tx’s Confirmation status with the new Confirmation status. I think we can just change it to:

if (wtxIn.tx_state.state != wtx.tx_state.state || wtxIn.tx_state.hashBlock != wtx.tx_state.hashBlock || wtxIn.tx_state.nIndex != wtx.tx_state.nIndex) {

ariard commented at 2:52 pm on August 21, 2019:

If these new asserts get triggered it would mean we have bugs in higher level code, but yes aborting maybe too strong given that this kind of bugs could be due to some likely API misuse in RPCs ? What’s about logging them with a warning ?

I’m not in favor of proposed alternative as it may cause hidden inconsistencies if these aforementioned high level bugs are present

ryanofsky commented at 4:35 pm on August 21, 2019:

I don’t feel strongly, but I think asserts are way to go because we should not allow bugs and uncertainty in wallet event processing code. It’s hard for me to imagine a scenario where it would be a win to use defensive programming in a place like this.

jnewbery commented at 4:54 pm on August 21, 2019:

@ryanofsky - do you agree that the scenario I describe above would trigger the assert?

ryanofsky commented at 7:18 pm on August 21, 2019:

Oh, I didn’t realize #16624 (review) was describing a specific scenario, not just a list of possible events. I could see that scenerio triggering these asserts, and making the change you suggested an improvement and bugfix. It is likely some change is needed here, and the suggested one seems ok.

More ideally, though, it’d be really nice to have a functional test that triggers these asserts, and instead of hiding the problem of an unconfirmed transaction being left in the CONFIRMED state after a reorg, there would be code on startup that marks reorged transactions UNCONFIRMED before rescanning (could basically be the same code from #15931 that looks up block heights on startup).

Nice catch though (assuming this a bug)!

jnewbery commented at 7:36 pm on August 21, 2019:

I didn’t realize #16624 (comment) was describing a specific scenario

sorry - changed my unordered list to an ordered list and updated wording to make this clearer.

jnewbery commented at 7:38 pm on August 21, 2019:

there would be code on startup that marks reorged transactions UNCONFIRMED before rescanning

Yes - I think if the wallet rescans from block at height x, it should first change all transactions that are CONFIRMED at height >x to UNCOFIRMED.

ariard commented at 9:32 pm on August 22, 2019:

I let asserts and move some bits from #15931 to transition status to UNCONFIRMED if blockHash not anymore in chain. Also added a wallet_reorgsrestore test with aforementioned scenario, without new code it triggers asserts

in src/wallet/wallet.h:480 in 8a7d26dc3c outdated

476@@ -477,16 +477,37 @@ class CWalletTx
477         fInMempool = false;
478         nChangeCached = 0;
479         nOrderPos = -1;
480+        tx_state.hashBlock = uint256();

ryanofsky commented at 4:29 pm on August 19, 2019:

In commit “Encapsulate tx state in a TxConfirmation struct” (8a7d26dc3c69e219a2db8d52980074951aff55a3)

It seems more safe to reset the whole struct instead of just one member. Maybe tx_state = TxConfirmation{};?

I think there may actually be a (theoretical) bug without this. If you unserialized an unconfirmed transaction into a CWalletTx that was not unconfirmed, deserialization code would not actually update the state to unconfirmed.

ariard commented at 5:27 pm on August 20, 2019:

Well strictly speaking, given that Init set to unconfirmed, it would be more wrongly updating to confirmed. But if hash is set IMO we have database corruption or bug in higher tracking logic. Updated serialization logic with this and beneath to make it more robust.

in src/wallet/wallet.h:552 in 8a7d26dc3c outdated

548+        if (serializedIndex == -1 && tx_state.hashBlock == ABANDON_HASH) {
549+            tx_state.nIndex = 0;
550+            tx_state.hashBlock = uint256();
551+            setAbandoned();
552+        } else if (serializedIndex == -1) {
553+            tx_state.nIndex = 0;

ryanofsky commented at 4:51 pm on August 19, 2019:

In commit “Remove SyncTransaction for conflicted txn in CWallet::BlockConnected” (3e027c776556c3070a9c3460045a6e397b21ceb8)

Can we drop these tx_state.nIndex = 0; assignments or set them in all cases consistently? (There is a missing assignment in the unconfirmed case). I’d have a slight preference for just dropping these and the Init() call above reset everything to default, instead of repeating same default values multiple places.

ariard force-pushed on Aug 19, 2019

ryanofsky approved

ryanofsky commented at 5:41 pm on August 19, 2019: member

utACK 3e027c776556c3070a9c3460045a6e397b21ceb8

ryanofsky approved

ryanofsky commented at 6:11 pm on August 19, 2019: member

utACK f9f492638a488149acceb7b6487553862e919028. Only change since last review is renaming tx_state to tx_conf, which is reasonable. I left some comments with my previous review, but they aren’t very important and are fine to ignore.

meshcollider commented at 5:38 am on August 20, 2019: contributor

Looks good to me, utACK f9f492638a488149acceb7b6487553862e919028

@ryanofsky My concern is that the new behavior might be less robust than previous behavior if there is a bug in higher level code that marks a transaction as confirmed in two different blocks without marking the transaction as unconfirmed in between.

I agree, I was looking at this specific change too. I think it should be fine but the additional check wouldn’t hurt :+1:

in src/wallet/test/wallet_tests.cpp:252 in f9f492638a outdated

248@@ -249,8 +249,9 @@ BOOST_FIXTURE_TEST_CASE(coin_mark_dirty_immature_credit, TestChain100Setup)
249     LockAssertion lock(::cs_main);
250     LOCK(wallet.cs_wallet);
251 
252-    wtx.hashBlock = ::ChainActive().Tip()->GetBlockHash();
253-    wtx.nIndex = 0;
254+    wtx.tx_conf.hashBlock = ::ChainActive().Tip()->GetBlockHash();

jnewbery commented at 5:23 pm on August 20, 2019:

Change to SetMerkleBranch()?

in src/wallet/rpcdump.cpp:387 in f9f492638a outdated

383@@ -384,8 +384,9 @@ UniValue importprunedfunds(const JSONRPCRequest& request)
384         throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Something wrong with merkleblock");
385     }
386 
387-    wtx.nIndex = txnIndex;
388-    wtx.hashBlock = merkleBlock.header.GetHash();
389+    wtx.tx_conf.nIndex = txnIndex;

jnewbery commented at 5:46 pm on August 20, 2019:

Change to SetMerkleBranch()?

in src/wallet/wallet.cpp:1419 in f9f492638a outdated

1413@@ -1425,11 +1414,10 @@ void CWallet::BlockConnected(const CBlock& block, const std::vector<CTransaction
1414     // the notification that the conflicted transaction was evicted.

jnewbery commented at 5:55 pm on August 20, 2019:

I think this whole comment can be removed (since you’re removing the SyncTransaction() call for the conflicting transactions). You could also move the for (const CTransactionRef& ptx : vtxConflicted) loop to below the for (size_t i = 0; i < block.vtx.size(); i++), which seems more natural.

in src/wallet/wallet.cpp:4627 in f9f492638a outdated

4625 
4626     // set the position of the transaction in the block
4627-    nIndex = posInBlock;
4628+    tx_conf.nIndex = posInBlock;
4629+
4630+    // Update tx state

jnewbery commented at 5:57 pm on August 20, 2019:

micronit: place this at the top of the function (so the order of logic matches the order of the arguments)

in src/wallet/wallet.cpp:4638 in f9f492638a outdated

4632 }
4633 
4634 int CWalletTx::GetDepthInMainChain(interfaces::Chain::Lock& locked_chain) const
4635 {
4636-    if (hashUnset())
4637+    if (isUnconfirmed() || isAbandoned())

jnewbery commented at 5:58 pm on August 20, 2019:

nit: could join this and the line below to one line to match current code style guide.

in src/wallet/wallet.h:485 in f9f492638a outdated

485-    /* An nIndex == -1 means that hashBlock (in nonzero) refers to the earliest
486-     * block in the chain we know this or any in-wallet dependency conflicts
487-     * with. Older clients interpret nIndex == -1 as unconfirmed for backward
488-     * compatibility.
489+
490+    /* New transactions start in the UNCONFIRMED. At BlockConnected,

jnewbery commented at 6:02 pm on August 20, 2019:

grammar nits:

s/start in the UNCONFIRMED/start as UNCONFIRMED/
s/they rollback/they roll back/
s/we update […] as CONFLICTED/we update […] to CONFLICTED/
s/In tx isn’t confirmed/If tx isn’t confirmed/
s/switch it to ABANDONED thanks to the abandontransaction call/switch it to ABANDONED by using the abandontransaction call/

in src/wallet/wallet.h:500 in f9f492638a outdated

500+        CONFIRMED,
501+        CONFLICTED,
502+        ABANDONED
503+    };
504+
505+    /* TxConfirmation includes tx state and a pair of block hash/block index at which tx has been confirmed.

jnewbery commented at 6:05 pm on August 20, 2019:

nit: I don’t think block index is well understood to mean ’the transaction’s position in the block’. I think explicitly calling it ‘a pair of {block hash, tx index in block}’ would be clearer.

in src/wallet/wallet.h:502 in f9f492638a outdated

502+        ABANDONED
503+    };
504+
505+    /* TxConfirmation includes tx state and a pair of block hash/block index at which tx has been confirmed.
506+     * This pair is both 0 if tx hasn't confirmed yet. Meaning of these fields changes with CONFLICTED state
507+     * where they instead point to block hash and index of the deepest conflicting tx.

jnewbery commented at 6:05 pm on August 20, 2019:

I don’t think this is right. When state is CONFLICTED, nIndex is set to 0

ariard commented at 2:30 pm on August 21, 2019:

Yes that’s right, I rewrote comment to new code without thinking it wasn’t relevant anymore

jnewbery commented at 7:07 pm on August 21, 2019:

Oh, I think you’ve removed too much now. It’s still useful to know that hashBlock refers to the block hash of the deepest conflicting transaction.

jnewbery commented at 3:21 pm on September 5, 2019:

This is still wrong. index is left as 0 for a CONFLICTED tx.

in src/wallet/wallet.h:507 in f9f492638a outdated

508      */
509-    int nIndex;
510+    struct TxConfirmation {
511+        uint256 hashBlock;
512+        int nIndex = 0;
513+        TxState state = UNCONFIRMED;

jnewbery commented at 6:07 pm on August 20, 2019:

nit: in the function calls, state comes before hashBlock and nIndex. I suggest you do the same here for consistency.

in src/wallet/wallet.h:510 in f9f492638a outdated

511+        uint256 hashBlock;
512+        int nIndex = 0;
513+        TxState state = UNCONFIRMED;
514+    };
515+
516+    TxConfirmation tx_conf;

jnewbery commented at 6:07 pm on August 20, 2019:

nit: code style guide says that data members should be named m_...

ariard commented at 2:32 pm on August 21, 2019:

Yes already corrected at 5ef9e95

in src/wallet/wallet.h:526 in f9f492638a outdated

522@@ -502,7 +523,7 @@ class CWalletTx
523         std::vector<char> dummy_vector1; //!< Used to be vMerkleBranch
524         std::vector<char> dummy_vector2; //!< Used to be vtxPrev
525         char dummy_char = false; //!< Used to be fSpent
526-        s << tx << hashBlock << dummy_vector1 << nIndex << dummy_vector2 << mapValueCopy << vOrderForm << fTimeReceivedIsTxTime << nTimeReceived << fFromMe << dummy_char;
527+        s << tx << (isAbandoned() ? ABANDON_HASH : tx_conf.hashBlock) << dummy_vector1 << ((isAbandoned() || isConflicted()) ? -1 : tx_conf.nIndex) << dummy_vector2 << mapValueCopy << vOrderForm << fTimeReceivedIsTxTime << nTimeReceived << fFromMe << dummy_char;

jnewbery commented at 6:10 pm on August 20, 2019:

nit: I’d prefer to not have these ternary operators in this very long line, since it makes it very difficult to read. I think it’d be clearer if you instantiated two local variables serialized_hash and serialized_index, and then use them in this serialization line.

in src/wallet/wallet.h:541 in f9f492638a outdated

533@@ -513,7 +534,27 @@ class CWalletTx
534         std::vector<uint256> dummy_vector1; //!< Used to be vMerkleBranch
535         std::vector<CMerkleTx> dummy_vector2; //!< Used to be vtxPrev
536         char dummy_char; //! Used to be fSpent
537-        s >> tx >> hashBlock >> dummy_vector1 >> nIndex >> dummy_vector2 >> mapValue >> vOrderForm >> fTimeReceivedIsTxTime >> nTimeReceived >> fFromMe >> dummy_char;
538+        int serializedIndex;
539+        s >> tx >> tx_conf.hashBlock >> dummy_vector1 >> serializedIndex >> dummy_vector2 >> mapValue >> vOrderForm >> fTimeReceivedIsTxTime >> nTimeReceived >> fFromMe >> dummy_char;
540+
541+        /* At serialization/deserialization, an nIndex == -1 means that hashBlock refers to
542+         * the earliest block in the chain we know this or any in-wallet dependency conflicts

jnewbery commented at 6:10 pm on August 20, 2019:

nit: should this be ‘any in-wallet ancestor’?

ariard commented at 2:41 pm on August 21, 2019:

Updated, just to be sure a ‘in-wallet dependency’ is relative to a parent tx on which it relies to be valid ?

in src/wallet/wallet.h:652 in f9f492638a outdated

647@@ -607,10 +648,12 @@ class CWalletTx
648      * >0 : is a coinbase transaction which matures in this many blocks
649      */
650     int GetBlocksToMaturity(interfaces::Chain::Lock& locked_chain) const;
651-    bool hashUnset() const { return (hashBlock.IsNull() || hashBlock == ABANDON_HASH); }
652-    bool isAbandoned() const { return (hashBlock == ABANDON_HASH); }
653-    void setAbandoned() { hashBlock = ABANDON_HASH; }
654-
655+    bool isAbandoned() const { return tx_conf.state == CWalletTx::ABANDONED; }
656+    void setAbandoned() { tx_conf.state = CWalletTx::ABANDONED; }

jnewbery commented at 6:30 pm on August 20, 2019:

consider updating this function to also zero out nIndex and hashBlock

ariard force-pushed on Aug 20, 2019

ariard commented at 6:57 pm on August 20, 2019: member

Updated at 5ef9e95 with @ryanofsky comments. Main changes are a more robust serialization logic and assert for already present txn in AddToWallet. This last one flagged a misbehavior of AddToWallet in ComputeTimeSmart (see commit message), so was worth adding.

in src/wallet/wallet.h:632 in 5ef9e9560b outdated

628@@ -590,7 +629,7 @@ class CWalletTx
629     // in place.
630     std::set<uint256> GetConflicts() const NO_THREAD_SAFETY_ANALYSIS;
631 
632-    void SetMerkleBranch(const uint256& block_hash, int posInBlock);
633+    void SetMerkleBranch(Status status, const uint256& block_hash, int posInBlock);

jnewbery commented at 7:42 pm on August 20, 2019:

nit: this function is badly named (and has been since 391dff16fe9ace90fc0f3308a5c63c453370e713 Do not store Merkle branches in the wallet.). Now would be a good time to rename it to SetTxConf() or similar.

jnewbery commented at 7:47 pm on August 20, 2019: member

Just nits so far. Still planning on doing a more thorough review.

DrahtBot added the label Needs rebase on Aug 21, 2019

ariard force-pushed on Aug 21, 2019

ariard commented at 3:19 pm on August 21, 2019: member

Rebased 63596cf with @jnewbery nits and comments addressed. Thanks for the review!

Still need to decide on #16624 (review) which spotted this #16624 (comment)

DrahtBot removed the label Needs rebase on Aug 21, 2019

ariard force-pushed on Aug 21, 2019

in src/wallet/wallet.cpp:1256 in e591ef7a79 outdated

1239@@ -1248,9 +1240,9 @@ bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef& ptx, const uint256
1240 
1241             CWalletTx wtx(this, ptx);
1242 
1243-            // Get merkle branch if transaction was found in a block
1244-            if (!block_hash.IsNull())
1245-                wtx.SetMerkleBranch(block_hash, posInBlock);
1246+            // Block disconnection override an abandoned tx as unconfirmed
1247+            // which means user may have to call abandontransaction again
1248+            wtx.SetConf(status, block_hash, posInBlock);

ryanofsky commented at 4:51 pm on August 21, 2019:

In commit “Encapsulate tx status in a Confirmation struct” (e591ef7a796d1ca164e791862e20a31b801fdf1b)

This is a little unclear, maybe replace “Block disconnection override an abandoned tx as unconfirmed” with “At block disconnection, this will change an abandoned transaction to be unconfirmed, whether or not the transaction is added back to the mempool.”

It might be better to move this comment to block disconnection code, since it really describing behavior of the caller, not this code directly, so it’s easy to imagine it becoming out of date.

Also it might be good if the comment said whether this is a real concern, or just a quirk, or how it might be addressed in the future (updating the state conditionally, adding a stickier abandoned state)?

ariard commented at 9:29 pm on August 22, 2019:

Yes I’ve tried to combine abandon state with every other one but would run into issue. IMO, would be better to rework abandontransaction to something like abandoncoins, because confirmation are on the transaction-level, but conflict/abandon at the output one

in src/wallet/wallet.h:506 in e591ef7a79 outdated

506+     * This pair is both 0 if tx hasn't confirmed yet.
507      */
508-    int nIndex;
509+    struct Confirmation {
510+        Status status = UNCONFIRMED;
511+        uint256 hashBlock = uint256();

ryanofsky commented at 5:06 pm on August 21, 2019:

In commit “Encapsulate tx status in a Confirmation struct” (e591ef7a796d1ca164e791862e20a31b801fdf1b)

Maybe add a doxygen comment for hashBlock saying it’s set to the block of the deepest conflicting transaction when the wallet transaction is in the CONFLICTED state. This information seems to have been lost in latest change to the comments above.

ariard commented at 9:29 pm on August 22, 2019:

Reestablished

ryanofsky approved

ryanofsky commented at 5:20 pm on August 21, 2019: member

utACK 63596cf1c56e1adb000f95cfce7aba5d96e838c2. Left some minor suggestions, feel free to ignore. Changes since last review: various field/method renames, tweaking setter methods to update multiple fields at once, comment updates, removing “TODO: Temporarily ensure” comment that I never understood, resetting all fields instead of just enum field in Init(), so the nIndex field is now reset during deserialization when transaction is unconfirmed.

ariard force-pushed on Aug 22, 2019

ariard commented at 9:33 pm on August 22, 2019: member

Updated at 4fc221d with new commit and test to solve #16624 (review)

ariard force-pushed on Aug 22, 2019

ariard force-pushed on Aug 23, 2019

Sjors commented at 10:50 am on August 23, 2019: member

Travis machines 5 and 8 will fail until the next rebase (see #16561).

MarcoFalke renamed this:
~~wallet : encapsulate transactions state~~
wallet: encapsulate transactions state
on Aug 23, 2019

ariard force-pushed on Aug 23, 2019

MarcoFalke commented at 5:41 pm on August 23, 2019: member

Travis should run the latest version and a rebase is not required.

Have you tried running the tests locally with ./configure --enable-debug?

MarcoFalke commented at 5:51 pm on August 23, 2019: member

0./out/x86_64-unknown-linux-gnu/bin/test_bitcoin -t wallet_tests
1Running 9 test cases...
2Assertion failed: detected inconsistent lock order at sync.cpp:121, details in debug log.
3unknown location(0): fatal error: in "wallet_tests/ComputeTimeSmart": signal: SIGABRT (application abort requested)
4wallet/test/wallet_tests.cpp(303): last checkpoint

MarcoFalke commented at 5:53 pm on August 23, 2019: member

 0
 1
 22019-08-23T17:36:07Z Bitcoin Core version v0.18.99.0-unk (debug build)
 32019-08-23T17:36:07Z Using 16 MiB out of 32/2 requested for signature cache, able to store 524288 elements
 42019-08-23T17:36:07Z Using 16 MiB out of 32/2 requested for script execution cache, able to store 524288 elements
 52019-08-23T17:36:07Z Opened LevelDB successfully
 62019-08-23T17:36:07Z Using obfuscation key for /tmp/test_common_Bitcoin Core/1566581767_852108204/blocks/index: 0000000000000000
 72019-08-23T17:36:07Z Opened LevelDB successfully
 82019-08-23T17:36:07Z Wrote new obfuscate key for /tmp/test_common_Bitcoin Core/1566581767_852108204/chainstate: ab46c080088c71f8
 92019-08-23T17:36:07Z Using obfuscation key for /tmp/test_common_Bitcoin Core/1566581767_852108204/chainstate: ab46c080088c71f8
102019-08-23T17:36:07Z Pre-allocating up to position 0x1000000 in blk00000.dat
112019-08-23T17:36:07Z UpdateTip: new best=000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f height=0 version=0x00000001 log2_work=32.000022 tx=1 date='2009-01-03T18:15:05Z' progress=0.000000 cache=0.0MiB(0txo)
122019-08-23T17:36:07Z ERROR: DeserializeFileDB: Failed to open file /tmp/test_common_Bitcoin Core/1566581767_852108204/banlist.dat
132019-08-23T17:36:07Z Invalid or missing banlist.dat; recreating
142019-08-23T17:36:07Z [default wallet] Wallet File Version = 10500
152019-08-23T17:36:07Z [default wallet] Keys: 0 plaintext, 0 encrypted, 0 w/ metadata, 0 total. Unknown wallet records: 0
162019-08-23T17:36:07Z (mocktime: 1970-01-01T00:01:40Z) POTENTIAL DEADLOCK DETECTED
172019-08-23T17:36:07Z (mocktime: 1970-01-01T00:01:40Z) Previous lock order was:
182019-08-23T17:36:07Z (mocktime: 1970-01-01T00:01:40Z)  (1) wallet.cs_wallet wallet/test/wallet_tests.cpp:283 (in thread )
192019-08-23T17:36:07Z (mocktime: 1970-01-01T00:01:40Z)  (1) cs_wallet wallet/wallet.cpp:1091 (in thread )
202019-08-23T17:36:07Z (mocktime: 1970-01-01T00:01:40Z)  (2) cs_main interfaces/chain.cpp:255 (in thread )
212019-08-23T17:36:07Z (mocktime: 1970-01-01T00:01:40Z) Current lock order is:
222019-08-23T17:36:07Z (mocktime: 1970-01-01T00:01:40Z)  (2) cs_main interfaces/chain.cpp:245 (in thread )
232019-08-23T17:36:07Z (mocktime: 1970-01-01T00:01:40Z)  (1) cs_wallet wallet/wallet.cpp:3352 (in thread )

Encapsulate tx status in a Confirmation struct

Instead of relying on combination of hashBlock and nIndex
values to manage tx in its lifecycle, we introduce 4
status : CONFIRMED, UNCONFIRMED, CONFLICTED, ABANDONED.

hashBlock and nIndex magic values should only be used at
serialization/deserialization for backward-compatibility.

At block disconnection, we know flag txn as UNCONFIRMED where
previously they kept their states until being override by a
block connection or abandontransaction call. This is a change
in behavior for which user may have to call abandon twice
if transaction is disconnected and not accepted back in the mempool.

We assert status transitioning right in AddToWallet. Doing so
flagged a misbehavior in ComputeTimeSmart unit test where same
tx is confirmed twice in different block. To avoid inconsistencies
we unconfirmed tx before new connection in different block. We
also remove a cs_main lock in test, as AddToWallet and its
callees don't rely on locked chain.

a31be09bfd

Remove SyncTransaction for conflicted txn in CWallet::BlockConnected

We shouldn't rely on this sync call to get an accurate view of txn
state, if a tx conflicts with one in mapTx we are going to update
our wallet dependencies in AddToWalletIfInvolvingMe while conflicting
txn get connected. If it doesn't conflict with one of our dependencies
we are not going to track it anyway.

This is a cleanup, as this SyncTransaction is redundant with the
following one for confirmation which is triggering the MarkConflicted
logic. We keep the loop because set of conflicted txn isn't same as txn
included in block.

7e89994133

ariard force-pushed on Aug 23, 2019

ariard commented at 8:03 pm on August 23, 2019: member

Updated at be2d6d3 with a lock order correction. Thanks to @MarcoFalke for spotting #16700 and finding the root issue with failures

in src/wallet/wallet.h:950 in 73b7aa0af1 outdated

945@@ -946,6 +946,9 @@ class CWallet final : public FillableSigningProvider, private interfaces::Chain:
946     bool IsLocked() const;
947     bool Lock();
948 
949+    /** Interface to assert chain access */
950+    bool hasChain() const { return m_chain != nullptr; }

ryanofsky commented at 8:54 pm on August 28, 2019:

In commit “Modify wallet tx status if has been reorged out” (73b7aa0af12c1110055c0d0c82415f7b92347217)

It’d be possible to avoid some repetition in this commit by adding a LockChain method instead of a HasChain method:

0std::unique_ptr<interfaces::Chain::Lock> LockChain() { return m_chain ? m_chain->lock() : nullptr; }

This would also allow shortening the “Even if we don’t use this lock…” comments by dropping the “If wallet doesn’t have a chain…” parts.

ariard commented at 4:07 pm on August 29, 2019:

Yes took this one, make it easier

in src/wallet/wallet.cpp:1184 in 73b7aa0af1 outdated

1181+    if (hasChain()) {
1182+        // If tx hasn't been reorged out of chain while wallet being shutdown
1183+        // change tx status to UNCONFIRMED and reset hashBlock/nIndex.
1184+        if (!wtxIn.m_confirm.hashBlock.IsNull()) {
1185+            auto locked_chain = chain().lock();
1186+            if (!locked_chain->getBlockHeight(wtxIn.m_confirm.hashBlock)) {

ryanofsky commented at 9:08 pm on August 28, 2019:

In commit “Modify wallet tx status if has been reorged out” (73b7aa0af12c1110055c0d0c82415f7b92347217)

Do you think that instead of calling getBlockHeight() and setUnconfirmed() immediately in LoadToWallet as individual transactions are loaded, the calls could happen later after transactions are loaded, in a loop over mapWallet from LoadWallet or CreateWalletFromFile? It seems like this could make locking simpler.

ariard commented at 4:11 pm on August 29, 2019:

It could and would make lock less a pain until #16426 but after it we will have the update status code lost in the middle of CreateWalletFromFile…not sure if it’s great to ease understanding of wallet bootstrap which is already intertwined a lot

in test/functional/wallet_reorgsrestore.py:6 in be2d6d3cd1 outdated

0@@ -0,0 +1,104 @@
1+#!/usr/bin/env python3
2+# Copyright (c) 2019 The Bitcoin Core developers
3+# Distributed under the MIT software license, see the accompanying
4+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
5+
6+"""Test tx status in case of reorgs while wallet being shutdown.

ryanofsky commented at 9:11 pm on August 28, 2019:

In commit “Add a test wallet_reorgsrestore” (be2d6d3cd1421d66871396eeef5f6e30b0841889)

Really nice to have this test!

in test/functional/wallet_reorgsrestore.py:89 in be2d6d3cd1 outdated

84+        self.nodes[1].sendrawtransaction(tx["hex"])
85+        self.nodes[1].generate(1)
86+        self.nodes[1].sendrawtransaction(conflicted["hex"])
87+        self.nodes[1].generate(1)
88+
89+        # Node0 wallet file is loaded on longuest sync'ed node1

ryanofsky commented at 9:12 pm on August 28, 2019:

In commit “Add a test wallet_reorgsrestore” (be2d6d3cd1421d66871396eeef5f6e30b0841889)

spelling longest

in test/functional/wallet_reorgsrestore.py:98 in be2d6d3cd1 outdated

92+        shutil.copyfile(os.path.join(self.nodes[0].datadir, 'wallet.bak'), os.path.join(self.nodes[1].datadir, 'regtest', 'wallet.dat'))
93+        self.start_node(1)
94+        tx_after_reorg = self.nodes[1].gettransaction(txid)
95+        # Check that normal confirmed tx is confirmed again but with different blockhash
96+        assert_equal(tx_after_reorg["confirmations"], 2)
97+        assert(tx_before_reorg["blockhash"] != tx_after_reorg["blockhash"])

ryanofsky commented at 9:17 pm on August 28, 2019:

In commit “Add a test wallet_reorgsrestore” (be2d6d3cd1421d66871396eeef5f6e30b0841889)

Not sure, but maybe it would be possible to use return values from generate to verify transactions are in the expected blocks, to be able to check more equality instead of just inequality. On the other hand, maybe the test is more robust this way, so feel free to leave this alone.

ariard commented at 4:08 pm on August 29, 2019:

I think the same effect is reached with confirmations depth assertations?

ryanofsky approved

ryanofsky commented at 9:29 pm on August 28, 2019: member

utACK be2d6d3cd1421d66871396eeef5f6e30b0841889. Only changes since last review are adding comments and adding back a removed test cs_main lock in the first commit, and adding third and fourth commits addressing reorg during shutdown bug #16624 (review)

I think locking changes in the third commit can be simplified and I left some review comments, but feel free to ignore them. I think the current approach is fine given that #16426 will get rid of this locking pretty much altogether.

Modify wallet tx status if has been reorged out

Add a LockChain method to CWallet to know if we can lock or query
chain state safely.

At tx loading, we rely on chain to know if hashBlock of tx is still
in main chain. If not, we set its status to unconfirmed and reset
its hashBlock/nIndex.

If wallet loaded is the wallet-tool one, all wallet txn will
show up with a height of zero. It doesn't matter as status is not
used by wallet-tool.

We take lock prematurely in CWallet::LoadWallet and CWallet::Verify
to ensure that lock order is respected between cs_main an cs_wallet.

40ede992d9

Add a test wallet_reorgsrestore

Test we change tx status at loading in case of reorgs while wallet
was shutdown.

442a87cc0a

ariard force-pushed on Aug 29, 2019

ariard commented at 4:07 pm on August 29, 2019: member

Updated at 442a87c with some @ryanofsky comments to take lock smoother. Note, also fix a race condition in the new wallet_reorgsrestore, were order of block announcements after block connection was making test successful or not.

in test/functional/wallet_reorgsrestore.py:69 in 442a87cc0a

64+        conflicting = self.nodes[0].signrawtransactionwithwallet(self.nodes[0].createrawtransaction(inputs, outputs_2))
65+
66+        conflicted_txid = self.nodes[0].sendrawtransaction(conflicted["hex"])
67+        self.nodes[0].generate(1)
68+        conflicting_txid = self.nodes[2].sendrawtransaction(conflicting["hex"])
69+        self.nodes[2].generate(9)

ryanofsky commented at 5:01 pm on August 29, 2019:

In commit “Add a test wallet_reorgsrestore” (442a87cc0ae43ebc9b6654a6165778eecb931f74)

It might be useful to add comments to explain where the 9 value comes from here and why the new (since 442a87cc0ae43ebc9b6654a6165778eecb931f74) sync_block call is necessary below. From #16624 (comment) it sounds like there was a race condition without these changes, so understanding this could save us from having to debug the same problem in the future in the course of maintaining or extending this test.

ryanofsky approved

ryanofsky commented at 5:06 pm on August 29, 2019: member

utACK 442a87cc0ae43ebc9b6654a6165778eecb931f74. Changes since last review are switching from hasChain to LockChain and removing chain lock in WalletBatch::LoadWallet that’s redundant with the new lock still added in CWallet::LoadWallet, and fixing python test race condition.

meshcollider commented at 1:27 pm on September 5, 2019: contributor

Light re-Code Review ACK 442a87cc0ae43ebc9b6654a6165778eecb931f74

I think this is in a good state, thank you @ryanofsky for being so active with review and @ariard for addressing feedback so quickly :)

meshcollider referenced this in commit 5e202382a9 on Sep 5, 2019

meshcollider merged this on Sep 5, 2019

meshcollider closed this on Sep 5, 2019

in src/wallet/wallet.cpp:1316 in 442a87cc0a

1312@@ -1310,7 +1313,7 @@ bool CWallet::AbandonTransaction(interfaces::Chain::Lock& locked_chain, const ui
1313         if (currentconfirm == 0 && !wtx.isAbandoned()) {
1314             // If the orig tx was not in block/mempool, none of its spends can be in mempool
1315             assert(!wtx.InMempool());
1316-            wtx.nIndex = -1;
1317+            wtx.m_confirm.nIndex = 0;

jnewbery commented at 3:22 pm on September 5, 2019:

This isn’t needed. setAbandoned() sets nIndex to 0.

in src/wallet/wallet.cpp:1181 in 442a87cc0a

1177 {
1178+    // If wallet doesn't have a chain (e.g wallet-tool), lock can't be taken.
1179+    auto locked_chain = LockChain();
1180+    // If tx hasn't been reorged out of chain while wallet being shutdown
1181+    // change tx status to UNCONFIRMED and reset hashBlock/nIndex.
1182+    if (!wtxIn.m_confirm.hashBlock.IsNull()) {

jnewbery commented at 3:36 pm on September 5, 2019:

nit: I think this would be clearer as:

0 if (wtxIn.isConflicted() || wtxIn.isConfirmed()) {
1    ...

jnewbery commented at 5:44 pm on September 5, 2019: member

ACK 442a87cc0ae43ebc9b6654a6165778eecb931f74 with a couple of nits inline.

Thanks for adding the test! An alternative approach to connecting/disconnecting and stop/starting the nodes would have been to use the loadwallet/unloadwallet and invalidateblock/reconsiderblock RPCs. I think that may have been clearer and faster to run.

sidhujag referenced this in commit 4c9cc729fc on Sep 10, 2019

ariard referenced this in commit f45e584e5d on Oct 14, 2019

ariard referenced this in commit 6593366d38 on Oct 14, 2019

ariard referenced this in commit de8fe7d717 on Oct 23, 2019

ariard referenced this in commit f5e55f7429 on Oct 24, 2019

ariard referenced this in commit 5014ebd2da on Oct 30, 2019

ariard referenced this in commit 93b69dfcc8 on Nov 6, 2019

ariard referenced this in commit 5971d3848e on Nov 6, 2019

meshcollider referenced this in commit 99ab3a72c5 on Nov 8, 2019

sidhujag referenced this in commit 16e27e9868 on Nov 9, 2019

laanwj referenced this in commit 312d27b11c on Mar 19, 2020

HashUnlimited referenced this in commit c4dd106efa on Apr 17, 2020

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

ryanofsky referenced this in commit f963a68051 on May 13, 2020

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

sidhujag referenced this in commit a84dd1fa2e on May 14, 2020

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

fanquake referenced this in commit ed0afe8c1f on May 15, 2020

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

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

ryanofsky referenced this in commit b604c5c8b5 on May 22, 2020

MarcoFalke referenced this in commit 3657aee2d2 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

deadalnix referenced this in commit 5e818a52f5 on Jul 28, 2020

deadalnix referenced this in commit 78c6a6547d on Jul 28, 2020

deadalnix referenced this in commit db8f22c063 on Jul 28, 2020

jasonbcox referenced this in commit 715386dc1f on Jul 28, 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

sidhujag referenced this in commit a22f1e2e6e on Nov 10, 2020

janus referenced this in commit f690d20f65 on Nov 15, 2020

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

in src/wallet/wallet.cpp:1422 in a31be09bfd outdated

1418@@ -1425,11 +1419,11 @@ void CWallet::BlockConnected(const CBlock& block, const std::vector<CTransaction
1419     // the notification that the conflicted transaction was evicted.
1420 
1421     for (const CTransactionRef& ptx : vtxConflicted) {
1422-        SyncTransaction(ptx, {} /* block hash */, 0 /* position in block */);
1423+        SyncTransaction(ptx, CWalletTx::Status::CONFLICTED, {} /* block hash */, 0 /* position in block */);

ryanofsky commented at 4:25 pm on January 7, 2021:

In commit “Encapsulate tx status in a Confirmation struct” (a31be09bfd77eed497a8e251d31358e16e2f2eb1)

This line is removed in the immediate next commit (https://github.com/bitcoin/bitcoin/pull/16624/commits/7e89994133725125dddbfa8d45484e3b9ed51c6e), but technically it has some bugs. Original intent of this code was only ever to send -walletnotifynotifications, not to update transaction state, and the change should have used UNCONFIRMED instead of CONFLICTED to stay equivalent to previous code. Problems with the change:

It was an undocumented change in behavior in what was supposed to be a refactoring commit
It forgot to pass the block hash to SyncTransaction so wasn’t properly setting a conflicted state. Instead it set an inconsistent state that would sometimes be treated as unconfirmed, sometimes conflicted.
It would compound reorg problems wallet already has treating transactions that are no longer conflicted as conflicted, see bug-stale-conflicted.

In the future it would actually be possible to mark these transactions as conflicted instead of unconfirmed. Way this could be done is described idea-more-conflicts.

in src/wallet/wallet.cpp:1440 in a31be09bfd outdated

1433@@ -1440,8 +1434,12 @@ void CWallet::BlockDisconnected(const CBlock& block) {
1434     auto locked_chain = chain().lock();
1435     LOCK(cs_wallet);
1436 
1437+    // At block disconnection, this will change an abandoned transaction to
1438+    // be unconfirmed, whether or not the transaction is added back to the mempool.
1439+    // User may have to call abandontransaction again. It may be addressed in the
1440+    // future with a stickier abandoned state or even removing abandontransaction call.

ryanofsky commented at 4:26 pm on January 7, 2021:

In commit “Encapsulate tx status in a Confirmation struct” (a31be09bfd77eed497a8e251d31358e16e2f2eb1)

Comment seems slightly off. I think the comment is right that abandontransaction would need to be called again in this case, but I don’t think it would just be because of this code. The abandoned state would already have been lost previously when the block was connected.

in src/wallet/wallet.cpp:1422 in 7e89994133 outdated

1417-    // notification of a connected conflict might cause an outside process
1418-    // to abandon a transaction and then have it inadvertently cleared by
1419-    // the notification that the conflicted transaction was evicted.
1420 
1421-    for (const CTransactionRef& ptx : vtxConflicted) {
1422-        SyncTransaction(ptx, CWalletTx::Status::CONFLICTED, {} /* block hash */, 0 /* position in block */);

ryanofsky commented at 4:27 pm on January 7, 2021:

In commit “Remove SyncTransaction for conflicted txn in CWallet::BlockConnected” (7e89994133725125dddbfa8d45484e3b9ed51c6e)

Dropping this SyncTransaction call was a bug, since it caused -walletnotify notifications to be lost. These were restored later in #18982, basically just reverting this commit, but with some updates due to notification code changes in the meantime, and using CONFIRMED instead of CONFLICTED to avoid a bug from the previous commit https://github.com/bitcoin/bitcoin/pull/16624/commits/a31be09bfd77eed497a8e251d31358e16e2f2eb1

ryanofsky commented at 4:39 pm on January 7, 2021: member

Note: After this PR was merged, some unexpected behavior changes were found. One bug caused, another one partially fixed. See 16624 in the history section this wiki page https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Transaction-Conflict-Tracking#history for details

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

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

Stackout referenced this in commit a2da6e0481 on Feb 17, 2022

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

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

kittywhiskers referenced this in commit 003c1cd8fc on Apr 3, 2022

kittywhiskers referenced this in commit 6b5a7c6e08 on Apr 3, 2022

kittywhiskers referenced this in commit 6c1f8b0013 on Apr 5, 2022

kittywhiskers referenced this in commit 9828801e56 on Apr 5, 2022

kittywhiskers referenced this in commit 8bfe16153b on Apr 6, 2022

kittywhiskers referenced this in commit f843acde16 on Apr 7, 2022

kittywhiskers referenced this in commit 1003efe293 on Apr 7, 2022

kittywhiskers referenced this in commit 74a97f6588 on Apr 17, 2022

kittywhiskers referenced this in commit ea3eefd30c on Apr 19, 2022

malbit referenced this in commit 72b925f69a on Aug 7, 2022

DrahtBot locked this on Aug 16, 2022

wallet: encapsulate transactions state #16624

Conflicts