Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove CValidationInterface::UpdatedTransaction #10178

Merged
merged 2 commits into from Apr 17, 2017

Conversation

Projects
None yet
8 participants
@TheBlueMatt
Copy link
Contributor

commented Apr 10, 2017

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.

@TheBlueMatt TheBlueMatt force-pushed the TheBlueMatt:2017-01-wallet-cache-inmempool-2 branch 2 times, most recently Apr 10, 2017

@fanquake fanquake added the Validation label Apr 10, 2017

src/wallet/wallet.cpp Outdated
if (mi != mapWallet.end())
NotifyTransactionChanged(this, hashPrevBestCoinbase, CT_UPDATED);
}
}

This comment has been minimized.

Copy link
@dcousens

dcousens Apr 10, 2017

Contributor

why is this in an extra block?

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Apr 10, 2017

Author Contributor

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

@TheBlueMatt TheBlueMatt force-pushed the TheBlueMatt:2017-01-wallet-cache-inmempool-2 branch Apr 10, 2017

@ryanofsky
Copy link
Contributor

left a comment

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

src/wallet/wallet.cpp Outdated
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.

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Apr 11, 2017

Contributor

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.

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Apr 11, 2017

Author Contributor

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.

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Apr 11, 2017

Contributor

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?

This comment has been minimized.

Copy link
@jnewbery

jnewbery Apr 11, 2017

Member

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.

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Apr 11, 2017

Author Contributor

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

src/wallet/wallet.cpp Outdated


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

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Apr 11, 2017

Contributor

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

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Apr 11, 2017

Author Contributor

Indeed, updated comment.

@TheBlueMatt TheBlueMatt force-pushed the TheBlueMatt:2017-01-wallet-cache-inmempool-2 branch Apr 11, 2017

@ryanofsky
Copy link
Contributor

left a comment

utACK 58d40125adfd05f8b5218a3141424821d426a162

src/wallet/wallet.cpp Outdated

// 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.

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Apr 11, 2017

Contributor

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.

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Apr 11, 2017

Author Contributor

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

@TheBlueMatt TheBlueMatt force-pushed the TheBlueMatt:2017-01-wallet-cache-inmempool-2 branch Apr 11, 2017

@jnewbery

This comment has been minimized.

Copy link
Member

commented Apr 11, 2017

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

@TheBlueMatt TheBlueMatt force-pushed the TheBlueMatt:2017-01-wallet-cache-inmempool-2 branch Apr 11, 2017

@jnewbery

This comment has been minimized.

Copy link
Member

commented Apr 11, 2017

utACK eeb2181b3dea071dde0d2aeb2e6a92cb53ee71c6

@ryanofsky

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2017

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
Copy link
Member

left a comment

utACK eeb2181b3dea071dde0d2aeb2e6a92cb53ee71c6

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

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar Apr 13, 2017

Member

style nit: curly braces for the one-line if

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Apr 13, 2017

Author Contributor

Fixed.

Remove CValidationInterface::UpdatedTransaction
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.

@TheBlueMatt TheBlueMatt force-pushed the TheBlueMatt:2017-01-wallet-cache-inmempool-2 branch to 9fececb Apr 13, 2017

@sdaftuar

This comment has been minimized.

Copy link
Member

commented Apr 13, 2017

re-utACK 9fececb

@laanwj

This comment has been minimized.

Copy link
Member

commented Apr 17, 2017

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

This comment has been minimized.

Copy link
Member

commented Apr 17, 2017

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

This comment has been minimized.

Copy link
Contributor Author

commented Apr 17, 2017

@laanwj

This comment has been minimized.

Copy link
Member

commented Apr 17, 2017

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.

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Apr 17, 2017

Merge #10178: Remove CValidationInterface::UpdatedTransaction
9fececb Remove CValidationInterface::UpdatedTransaction (Matt Corallo)
d89f8ad Make DisconnectBlock and ConnectBlock static in validation.cpp (Matt Corallo)

Tree-SHA512: 146597b538c09c1e8071f4f88ffeba0645c6816f86030b142174bd298cc18ae09a400e6ca8de04d091e37b635f99f4c05982c09e6729691eb8ca6b8439ab97ca

laanwj added a commit that referenced this pull request Nov 15, 2017

Merge #10286: Call wallet notify callbacks in scheduler thread (witho…
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.