Remove CValidationInterface::UpdatedTransaction #10178

pull TheBlueMatt wants to merge 2 commits into bitcoin:master from TheBlueMatt:2017-01-wallet-cache-inmempool-2 changing 6 files +41 −40
  1. TheBlueMatt commented at 6:47 pm on April 10, 2017: member

    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.

  2. Make DisconnectBlock and ConnectBlock static in validation.cpp d89f8adf25
  3. TheBlueMatt force-pushed on Apr 10, 2017
  4. TheBlueMatt force-pushed on Apr 10, 2017
  5. fanquake added the label Validation on Apr 10, 2017
  6. dcousens approved
  7. in src/wallet/wallet.cpp:1176 in b6d4fc2715 outdated 
    1175+            } else {
1176+                std::map<uint256, CWalletTx>::const_iterator mi = mapWallet.find(hashPrevBestCoinbase);
1177+                if (mi != mapWallet.end())
1178+                    NotifyTransactionChanged(this, hashPrevBestCoinbase, CT_UPDATED);
1179+            }
1180+        }
    dcousens commented at 11:03 pm on April 10, 2017:
    why is this in an extra block?
    TheBlueMatt commented at 11:08 pm on April 10, 2017:
    TBH I have no idea…probably copied and had a lock block, anyway, removed.
  8. TheBlueMatt force-pushed on Apr 10, 2017
  9. in src/wallet/wallet.cpp:1165 in 9f51ad1300 outdated 
    1160+    if (pindex) {
1161+        // Only notify UI if this transaction is in this wallet
1162+        if (hashPrevBestCoinbase.IsNull()) {
1163+            // For correctness we scan over the entire wallet, looking for
1164+            // the previous block's coinbase, just in case it is ours, so
1165+            // that we can notify the UI that it should now be displayed.
    ryanofsky commented at 1:48 pm on April 11, 2017:

    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:

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

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

    TheBlueMatt commented at 2:32 pm on April 11, 2017:

    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.

    ryanofsky commented at 2:56 pm on April 11, 2017:

    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?

    jnewbery commented at 3:09 pm on April 11, 2017:

    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:

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

    seems clearer to me.

    TheBlueMatt commented at 3:35 pm on April 11, 2017:
    OK, I completely rewrote the commit message, better now?
  10. in src/wallet/wallet.cpp:1161 in 9f51ad1300 outdated 
    1154@@ -1155,6 +1155,30 @@ void CWallet::BlockConnected(const std::shared_ptr<const CBlock>& pblock, const
1155     for (size_t i = 0; i < pblock->vtx.size(); i++) {
1156         SyncTransaction(pblock->vtx[i], pindex, i);
1157     }
1158+
1159+
1160+    if (pindex) {
1161+        // Only notify UI if this transaction is in this wallet
    ryanofsky commented at 2:02 pm on April 11, 2017:
    Copy and paste error? There is no “this transaction” at this point.
    TheBlueMatt commented at 2:30 pm on April 11, 2017:
    Indeed, updated comment.
  11. ryanofsky commented at 2:25 pm on April 11, 2017: member
    utACK 9f51ad130049fa5cbcd83d5bc5e0d145d6a73cc3. Code seems perfectly fine, though I found the comments confusing.
  12. TheBlueMatt force-pushed on Apr 11, 2017
  13. in src/wallet/wallet.cpp:1161 in 58d40125ad outdated 
    1154@@ -1155,6 +1155,31 @@ void CWallet::BlockConnected(const std::shared_ptr<const CBlock>& pblock, const
1155     for (size_t i = 0; i < pblock->vtx.size(); i++) {
1156         SyncTransaction(pblock->vtx[i], pindex, i);
1157     }
1158+
1159+    // The GUI expects a NotifyTransactionChanged when a coinbase tx
1160+    // which is in our wallet moves from in-the-best-block to
1161+    // 2-confirmations. We do that here.
    ryanofsky commented at 2:42 pm on April 11, 2017:
    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.
    TheBlueMatt commented at 2:58 pm on April 11, 2017:
    Updated the comment, the GUI simply doesnt display such transactions until they are buried.
  14. ryanofsky commented at 2:50 pm on April 11, 2017: member
    utACK 58d40125adfd05f8b5218a3141424821d426a162
  15. TheBlueMatt force-pushed on Apr 11, 2017
  16. jnewbery commented at 3:09 pm on April 11, 2017: member
    utACK 541ceeb0ef416a87dbd3d07f370950826eca87b9, modulo @ryanofsky’s style/comment nits.
  17. TheBlueMatt force-pushed on Apr 11, 2017
  18. jnewbery commented at 4:49 pm on April 11, 2017: member
    utACK eeb2181b3dea071dde0d2aeb2e6a92cb53ee71c6
  19. ryanofsky commented at 7:21 pm on April 11, 2017: member

    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.

  20. in src/wallet/wallet.cpp:1179 in eeb2181b3d outdated 
    1174+                }
1175+            }
1176+        }
1177+    } else {
1178+        std::map<uint256, CWalletTx>::const_iterator mi = mapWallet.find(hashPrevBestCoinbase);
1179+        if (mi != mapWallet.end())
    sdaftuar commented at 2:13 pm on April 13, 2017:
    style nit: curly braces for the one-line if
    TheBlueMatt commented at 2:36 pm on April 13, 2017:
    Fixed.
  21. sdaftuar approved
  22. sdaftuar commented at 2:25 pm on April 13, 2017: member
    utACK eeb2181b3dea071dde0d2aeb2e6a92cb53ee71c6
  23. 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.
    9fececb2cb
  24. TheBlueMatt force-pushed on Apr 13, 2017
  25. sdaftuar commented at 3:15 pm on April 13, 2017: member
    re-utACK 9fececb
  26. laanwj commented at 7:35 am on April 17, 2017: member

    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

  27. sipa commented at 11:22 am on April 17, 2017: member

    utACK 9fececb2cbabc52cc375b84bf840fac018cc8121

    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?

  28. TheBlueMatt commented at 12:24 pm on April 17, 2017: member

    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” notifications@github.com 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.

  29. laanwj commented at 12:45 pm on April 17, 2017: member

    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.

  30. laanwj merged this on Apr 17, 2017
  31. laanwj closed this on Apr 17, 2017
  32. laanwj referenced this in commit 2584925077 on Apr 17, 2017
  33. laanwj referenced this in commit 927a1d7d08 on Nov 15, 2017
  34. PastaPastaPasta referenced this in commit 246153a2a6 on May 31, 2019
  35. PastaPastaPasta referenced this in commit 325676fac7 on May 31, 2019
  36. PastaPastaPasta referenced this in commit 1bc99efd9b on Jun 10, 2019
  37. PastaPastaPasta referenced this in commit 1d7ac2b65b on Jun 10, 2019
  38. PastaPastaPasta referenced this in commit 4584b573ce on Jun 11, 2019
  39. PastaPastaPasta referenced this in commit f46f06251f on Jun 11, 2019
  40. PastaPastaPasta referenced this in commit 543ce8bc0f on Jun 12, 2019
  41. PastaPastaPasta referenced this in commit d28cbd8357 on Jun 15, 2019
  42. PastaPastaPasta referenced this in commit 54959bbcce on Jun 19, 2019
  43. PastaPastaPasta referenced this in commit 889be06e11 on Jun 19, 2019
  44. PastaPastaPasta referenced this in commit 6643720579 on Jun 19, 2019
  45. PastaPastaPasta referenced this in commit 61758874b8 on Feb 13, 2020
  46. PastaPastaPasta referenced this in commit 76ed385c03 on Feb 13, 2020
  47. PastaPastaPasta referenced this in commit 3eb80fdcd5 on Feb 29, 2020
  48. PastaPastaPasta referenced this in commit 803530c83d on Feb 29, 2020
  49. random-zebra referenced this in commit 2e510ef586 on Feb 18, 2021
  50. ckti referenced this in commit 38a9ad2b41 on Mar 28, 2021
  51. gades referenced this in commit 1459aa5a72 on Jun 30, 2021
  52. DrahtBot locked this on Sep 8, 2021


TheBlueMatt dcousens ryanofsky jnewbery sdaftuar laanwj sipa

Labels
Validation

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-18 18:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me