Remove CValidationInterface::UpdatedTransaction #10178

TheBlueMatt · 2017-04-10T18:47:53Z

Based on #9725. Also bonus of getting two extra static declarations in.

This removes another callback from block connection logic, making it
easier to reason about the wallet-RPCs-returns-stale-info issue.

Note that this does a full mapWallet loop after the first block
which is connected after restart. This is due to a previous bug
where a coinbase transaction from the current best tip which
existed in wallet immediately prior to restart would not be shown
in the GUI except because on-load -checkblocks would call
DisconnectBlock and ConnectBlock on the current tip. To avoid
making it worse (ie that such transactions would never be displayed
until restart), a scan to find such transactions was added.

dcousens · 2017-04-10T23:03:14Z

src/wallet/wallet.cpp

+                if (mi != mapWallet.end())
+                    NotifyTransactionChanged(this, hashPrevBestCoinbase, CT_UPDATED);
+            }
+        }


why is this in an extra block?

TBH I have no idea...probably copied and had a lock block, anyway, removed.

ryanofsky

utACK 9f51ad130049fa5cbcd83d5bc5e0d145d6a73cc3. Code seems perfectly fine, though I found the comments confusing.

ryanofsky · 2017-04-11T13:48:57Z

src/wallet/wallet.cpp

+        if (hashPrevBestCoinbase.IsNull()) {
+            // For correctness we scan over the entire wallet, looking for
+            // the previous block's coinbase, just in case it is ours, so
+            // that we can notify the UI that it should now be displayed.


I found this code and comment, as well as the comment in the commit description really confusing (I think there are some words missing between "except because" in the commit description?). I don't get why it's important to call NotifyTransactionChanged on the coinbase transaction of the previous block when a new block is added.

I would also suggest structuring the code as:

if (hashPrevBestCoinbase.IsNull()) { ... loop hunting for hashPrevBestCoinbase in mapWallet ... } if (!hashPrevBestCoinbase.IsNull() && mapWallet.count(hashPrevBestCoinbase)) { // Notify UI about coinbase transaction in the previous block when a new block // is added because [reason]. NotifyTransactionChanged(this, hashPrevBestCoinbase, CT_UPDATED); } hashPrevBestCoinbase = pblock->vtx[0]->GetHash();

Code change is just a suggestion, but I think the comment really should be improved.

I expanded a bit on the comment. The commit message is correct, if its hard to read, any suggestions for improvements?

I removed the outer-level if statement, as it is now useless, indeed.

The commit message is correct, if its hard to read, any suggestions for improvements?

I still don't understand the commit message so I can't say how to improve it. But here is how it parses to me:

Note that this does a full mapWallet loop after the first block
which is connected after restart.

Fine so far.

This is due to a previous bug where a coinbase transaction from the current best tip which existed in wallet immediately prior to restart would not be shown in the GUI

How is a previous bug relevant? Was it not fixed? What does "existed immediately prior to restart" mean? Does it mean a transaction added immediately prior to restart? When would the transaction not be shown in the GUI, before or after the restart?

except because on-load -checkblocks would call DisconnectBlock and ConnectBlock on the current tip.

Are there words missing between "except" and "because"? Is this saying the transaction would show up after the restart but not before?

To avoid making it worse (ie that such transactions would never be displayed
until restart), a scan to find such transactions was added.

Is this referring to the scan you are adding in this commit, or some other scan?

I agree with @ryanofsky's suggested code change. That structure seems clearer to me.

I think the commit message can be shortened and the reference to the previous bug removed. Something like:

This does a full mapWallet loop after the first block is connected after restart. This is so NotifyTransactionChanged() is called for the coinbase transaction of the previous best block if it belongs to us.

seems clearer to me.

OK, I completely rewrote the commit message, better now?

ryanofsky · 2017-04-11T14:02:47Z

src/wallet/wallet.cpp

+
+
+    if (pindex) {
+        // Only notify UI if this transaction is in this wallet


Copy and paste error? There is no "this transaction" at this point.

Indeed, updated comment.

ryanofsky

utACK 58d40125adfd05f8b5218a3141424821d426a162

ryanofsky · 2017-04-11T14:42:06Z

src/wallet/wallet.cpp

+
+    // The GUI expects a NotifyTransactionChanged when a coinbase tx
+    // which is in our wallet moves from in-the-best-block to
+    // 2-confirmations. We do that here.


Thanks, this is much better. I still have no idea why the GUI cares specifically about the second confirmation of the coinbase transaction in the previous block. Why not other transactions in the previous block? If you could add another sentence here suggesting a reason, that might be helpful.

Updated the comment, the GUI simply doesnt display such transactions until they are buried.

jnewbery · 2017-04-11T15:09:54Z

utACK 541ceeb0ef416a87dbd3d07f370950826eca87b9, modulo @ryanofsky's style/comment nits.

jnewbery · 2017-04-11T16:49:41Z

utACK eeb2181b3dea071dde0d2aeb2e6a92cb53ee71c6

ryanofsky · 2017-04-11T19:21:21Z

utACK eeb2181b3dea071dde0d2aeb2e6a92cb53ee71c6. Would still suggest making some edits to the description to make it clear what exact corner case was broken before and now fixed:

UpdatedTransaction was previously ~~~used by~~~ called to cause the GUI to display
coinbase transactions ~~~only after they have~~~ after having a block built on top of
them. This worked fine ~~~for~~~ in most cases, but ~~~only worked due to a~~~ not in the
corner case if the user received a coinbase payout in a block
immediately prior to restart ~~~. In that case, the normal process of
caching the most recent coinbase transaction's hash would not work,
and instead it would only work because of~~~ , and then restarted with -checkblocks=0 (which would disable the on-load -checkblocks
calling DisconnectBlock and ConnectBlock on the current tip. )

In order to make this more robust, a full mapWallet loop after the
first block which is connected after restart ~~~was~~~ is now added.

sdaftuar

utACK eeb2181b3dea071dde0d2aeb2e6a92cb53ee71c6

sdaftuar · 2017-04-13T14:13:17Z

src/wallet/wallet.cpp

+        }
+    } else {
+        std::map<uint256, CWalletTx>::const_iterator mi = mapWallet.find(hashPrevBestCoinbase);
+        if (mi != mapWallet.end())


style nit: curly braces for the one-line if

This removes another callback from block connection logic, making it easier to reason about the wallet-RPCs-returns-stale-info issue. UpdatedTransaction was previously used by the GUI to display coinbase transactions only after they have a block built on top of them. This worked fine for in most cases, but only worked due to a corner case if the user received a coinbase payout in a block immediately prior to restart. In that case, the normal process of caching the most recent coinbase transaction's hash would not work, and instead it would only work because of the on-load -checkblocks calling DisconnectBlock and ConnectBlock on the current tip. In order to make this more robust, a full mapWallet loop after the first block which is connected after restart was added.

sdaftuar · 2017-04-13T15:15:19Z

re-utACK 9fececb

laanwj · 2017-04-17T07:35:51Z

I think it would be good to get rid of the GUI logic that only displays generated transactions at confirmations=2. It is a left-over from the times that it was still realistic to mine locally in the GUI, and needlessly complicates this.

utACK 9fececb otherwise

sipa · 2017-04-17T11:22:04Z

utACK 9fececb

I think it would be good to get rid of the GUI logic that only displays generated transactions at confirmations=2. It is a left-over from the times that it was still realistic to mine locally in the GUI, and needlessly complicates this.

That can be done as a follow-up PR?

TheBlueMatt · 2017-04-17T12:24:53Z

Indeed, I didn't want to get into a discussion on changing the GUI to simplify internals, so left that to a separate PR. I'd say this is still pretty simple, but I'm happy to take a look at removing the display quirk if you prefer.

…

On April 17, 2017 3:35:56 AM EDT, "Wladimir J. van der Laan" ***@***.***> wrote: I think it would be good to get rid of the GUI logic that only displays generated transactions at confirmations=2. It is a left-over from the times that it was still realistic to mine locally in the GUI, and needlessly complicates this.

laanwj · 2017-04-17T12:45:23Z

Indeed, I didn't want to get into a discussion on changing the GUI to simplify internals

Not just to simplify internals, but it's a (virtually) untested code path. Also the core side shouldn't have to be doing this kind of acrobatics specifically to preserve a GUI quirk. If this is worth keeping it should be handled at the side of the GUI.

That can be done as a follow-up PR?

Of course.

9fececb Remove CValidationInterface::UpdatedTransaction (Matt Corallo) d89f8ad Make DisconnectBlock and ConnectBlock static in validation.cpp (Matt Corallo) Tree-SHA512: 146597b538c09c1e8071f4f88ffeba0645c6816f86030b142174bd298cc18ae09a400e6ca8de04d091e37b635f99f4c05982c09e6729691eb8ca6b8439ab97ca

…ut cs_main) 89f0312 Remove redundant pwallet nullptr check (Matt Corallo) c4784b5 Add a dev notes document describing the new wallet RPC blocking (Matt Corallo) 3ea8b75 Give ZMQ consistent order with UpdatedBlockTip on scheduler thread (Matt Corallo) cb06edf Fix wallet RPC race by waiting for callbacks in sendrawtransaction (Matt Corallo) e545ded Also call other wallet notify callbacks in scheduler thread (Matt Corallo) 17220d6 Use callbacks to cache whether wallet transactions are in mempool (Matt Corallo) 5d67a78 Add calls to CWallet::BlockUntilSyncedToCurrentChain() in RPCs (Matt Corallo) 5ee3172 Add CWallet::BlockUntilSyncedToCurrentChain() (Matt Corallo) 0b2f42d Add CallFunctionInQueue to wait on validation interface queue drain (Matt Corallo) 2b4b345 Add ability to assert a lock is not held in DEBUG_LOCKORDER (Matt Corallo) 0343676 Call TransactionRemovedFromMempool in the CScheduler thread (Matt Corallo) a7d3936 Add a CValidationInterface::TransactionRemovedFromMempool (Matt Corallo) Pull request description: Based on #10179, this effectively reverts #9583, regaining most of the original speedups of #7946. This concludes the work of #9725, #10178, and #10179. See individual commit messages for more information. Tree-SHA512: eead4809b0a75d1fb33b0765174ff52c972e45040635e38cf3686cef310859c1e6b3c00e7186cbd17374c6ae547bfbd6c1718fe36f26c76ba8a8b052d6ed7bc9

9fececb Remove CValidationInterface::UpdatedTransaction (Matt Corallo) d89f8ad Make DisconnectBlock and ConnectBlock static in validation.cpp (Matt Corallo) Tree-SHA512: 146597b538c09c1e8071f4f88ffeba0645c6816f86030b142174bd298cc18ae09a400e6ca8de04d091e37b635f99f4c05982c09e6729691eb8ca6b8439ab97ca

9fececb Remove CValidationInterface::UpdatedTransaction (Matt Corallo) d89f8ad Make DisconnectBlock and ConnectBlock static in validation.cpp (Matt Corallo) Tree-SHA512: 146597b538c09c1e8071f4f88ffeba0645c6816f86030b142174bd298cc18ae09a400e6ca8de04d091e37b635f99f4c05982c09e6729691eb8ca6b8439ab97ca undo unintended change

… (without cs_main) 89f0312 Remove redundant pwallet nullptr check (Matt Corallo) c4784b5 Add a dev notes document describing the new wallet RPC blocking (Matt Corallo) 3ea8b75 Give ZMQ consistent order with UpdatedBlockTip on scheduler thread (Matt Corallo) cb06edf Fix wallet RPC race by waiting for callbacks in sendrawtransaction (Matt Corallo) e545ded Also call other wallet notify callbacks in scheduler thread (Matt Corallo) 17220d6 Use callbacks to cache whether wallet transactions are in mempool (Matt Corallo) 5d67a78 Add calls to CWallet::BlockUntilSyncedToCurrentChain() in RPCs (Matt Corallo) 5ee3172 Add CWallet::BlockUntilSyncedToCurrentChain() (Matt Corallo) 0b2f42d Add CallFunctionInQueue to wait on validation interface queue drain (Matt Corallo) 2b4b345 Add ability to assert a lock is not held in DEBUG_LOCKORDER (Matt Corallo) 0343676 Call TransactionRemovedFromMempool in the CScheduler thread (Matt Corallo) a7d3936 Add a CValidationInterface::TransactionRemovedFromMempool (Matt Corallo) Pull request description: Based on bitcoin#10179, this effectively reverts bitcoin#9583, regaining most of the original speedups of bitcoin#7946. This concludes the work of bitcoin#9725, bitcoin#10178, and bitcoin#10179. See individual commit messages for more information. Tree-SHA512: eead4809b0a75d1fb33b0765174ff52c972e45040635e38cf3686cef310859c1e6b3c00e7186cbd17374c6ae547bfbd6c1718fe36f26c76ba8a8b052d6ed7bc9

…background thread 6cecb7b AddToWalletIfInvolvingMe should test pIndex, not posInBlock (furszy) 73499ff Add missing lock in CScheduler::AreThreadsServicingQueue() (Matt Corallo) 6d1a60e ValidationInterface compiler warning fix (furszy) 9e523a7 Fix shutdown in case of errors during initialization (Wladimir J. van der Laan) 207c17f Expose if CScheduler is being serviced, assert its not in EmptyQueue (Matt Corallo) 64c525b Otherwise threads will try to continue modifying data structures while the dump process is being executed. (Matt Corallo) 8022463 Support more than one CScheduler thread for serial clients (Matt Corallo) 6532fca Add default arg to CScheduler to schedule() a callback now (Matt Corallo) d583d12 Give CMainSignals a reference to the global scheduler so that it can run some signals in the background later (Matt Corallo) c26d18c Remove unused NotifyTransactionLock signal. (furszy) 2c9de45 Remove unused UpdatedTransaction signal and connected slot. (furszy) 8f38657 make ConnectBlock static in validation.cpp (furszy) Pull request description: Another step forward in the wallet signals processing in the background thread + node validation <--> wallet separation long working paths. Adapting: * bitcoin#10178 * bitcoin#10179 * bitcoin#10221 * bitcoin#10914 * bitcoin#11783 * Plus added an extra cleanup to the validation interface removing unused `NotifyTransactionLock` signal and its respective slots. ACKs for top commit: Fuzzbawls: ACK 6cecb7b random-zebra: ACK 6cecb7b and merging... Tree-SHA512: ac1c128cb3e4ec47c2803da68ca4d4b16164687fc790f3cc1d0761e73bd61b4edc681e2ce045e52564dd13a8279495e9058a6aae316196bd7ad9c97a07dd0b20

… (without cs_main) 89f0312 Remove redundant pwallet nullptr check (Matt Corallo) c4784b5 Add a dev notes document describing the new wallet RPC blocking (Matt Corallo) 3ea8b75 Give ZMQ consistent order with UpdatedBlockTip on scheduler thread (Matt Corallo) cb06edf Fix wallet RPC race by waiting for callbacks in sendrawtransaction (Matt Corallo) e545ded Also call other wallet notify callbacks in scheduler thread (Matt Corallo) 17220d6 Use callbacks to cache whether wallet transactions are in mempool (Matt Corallo) 5d67a78 Add calls to CWallet::BlockUntilSyncedToCurrentChain() in RPCs (Matt Corallo) 5ee3172 Add CWallet::BlockUntilSyncedToCurrentChain() (Matt Corallo) 0b2f42d Add CallFunctionInQueue to wait on validation interface queue drain (Matt Corallo) 2b4b345 Add ability to assert a lock is not held in DEBUG_LOCKORDER (Matt Corallo) 0343676 Call TransactionRemovedFromMempool in the CScheduler thread (Matt Corallo) a7d3936 Add a CValidationInterface::TransactionRemovedFromMempool (Matt Corallo) Pull request description: Based on bitcoin#10179, this effectively reverts bitcoin#9583, regaining most of the original speedups of bitcoin#7946. This concludes the work of bitcoin#9725, bitcoin#10178, and bitcoin#10179. See individual commit messages for more information. Tree-SHA512: eead4809b0a75d1fb33b0765174ff52c972e45040635e38cf3686cef310859c1e6b3c00e7186cbd17374c6ae547bfbd6c1718fe36f26c76ba8a8b052d6ed7bc9

Make DisconnectBlock and ConnectBlock static in validation.cpp

d89f8ad

TheBlueMatt force-pushed the 2017-01-wallet-cache-inmempool-2 branch 2 times, most recently from 04c754a to b6d4fc2 Compare April 10, 2017 20:25

TheBlueMatt mentioned this pull request Apr 10, 2017

Give CValidationInterface Support for calling notifications on the CScheduler Thread #10179

Merged

fanquake added the Validation label Apr 10, 2017

dcousens approved these changes Apr 10, 2017

View reviewed changes

dcousens reviewed Apr 10, 2017

View reviewed changes

TheBlueMatt force-pushed the 2017-01-wallet-cache-inmempool-2 branch from b6d4fc2 to 9f51ad1 Compare April 10, 2017 23:09

ryanofsky reviewed Apr 11, 2017

View reviewed changes

TheBlueMatt force-pushed the 2017-01-wallet-cache-inmempool-2 branch from 9f51ad1 to 58d4012 Compare April 11, 2017 14:32

ryanofsky reviewed Apr 11, 2017

View reviewed changes

TheBlueMatt force-pushed the 2017-01-wallet-cache-inmempool-2 branch from 58d4012 to 541ceeb Compare April 11, 2017 14:57

TheBlueMatt force-pushed the 2017-01-wallet-cache-inmempool-2 branch from 541ceeb to eeb2181 Compare April 11, 2017 15:35

sdaftuar approved these changes Apr 13, 2017

View reviewed changes

TheBlueMatt force-pushed the 2017-01-wallet-cache-inmempool-2 branch from eeb2181 to 9fececb Compare April 13, 2017 14:36

laanwj merged commit 9fececb into bitcoin:master Apr 17, 2017

TheBlueMatt mentioned this pull request Apr 17, 2017

Stop treating coinbase outputs differently in GUI: show them at 1conf #10221

Merged

TheBlueMatt mentioned this pull request Apr 27, 2017

Call wallet notify callbacks in scheduler thread (without cs_main) #10286

Merged

codablock mentioned this pull request Jun 13, 2019

Backports 0.15 pr14 dashpay/dash#2974

Merged

furszy mentioned this pull request Feb 6, 2021

Support for validation interface signals in background thread PIVX-Project/PIVX#2192

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021



		if (pindex) {
		// Only notify UI if this transaction is in this wallet

Remove CValidationInterface::UpdatedTransaction #10178

Remove CValidationInterface::UpdatedTransaction #10178

Uh oh!

Conversation

TheBlueMatt commented Apr 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Apr 11, 2017

Uh oh!

jnewbery commented Apr 11, 2017

Uh oh!

ryanofsky commented Apr 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sdaftuar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sdaftuar commented Apr 13, 2017

Uh oh!

laanwj commented Apr 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented Apr 17, 2017

Uh oh!

TheBlueMatt commented Apr 17, 2017 via email

Uh oh!

laanwj commented Apr 17, 2017

Uh oh!

Uh oh!

TheBlueMatt commented Apr 10, 2017 •

edited

Loading

ryanofsky commented Apr 11, 2017 •

edited

Loading

laanwj commented Apr 17, 2017 •

edited

Loading