Remove the mempool’s NotifyEntryAdded and NotifyEntryRemoved signals #17477

pull jnewbery wants to merge 6 commits into bitcoin:master from jnewbery:2019-11-remove-mempool-signals2 changing 18 files +58 −65
  1. jnewbery commented at 4:21 pm on November 14, 2019: member

    These boost signals were added in #9371, before we had a TransactionRemovedFromMempool method in the validation interface. The NotifyEntryAdded callback was used by validation to build a vector of conflicted transactions when connecting a block, which the wallet was notified of in the BlockConnected CValidationInterface callback.

    Now that we have a TransactionRemovedFromMempool callback, we can fire that signal directly from the mempool for conflicted transactions.

    Note that #9371 was implemented to ensure -walletnotify events were fired for these conflicted transaction. We inadvertently stopped sending these notifications in #16624 (Sep 2019 commit 7e89994). We should probably fix that, but in a different PR.

  2. jnewbery commented at 4:21 pm on November 14, 2019: member
    Request review from @TheBlueMatt and @morcos @TheBlueMatt - you introduced the vtxConflicted vector in BlockConnected in 461e49fee2935b1eb4d4ea7bae3023e655c0a6d8 with comment ‘This change adds a parameter to BlockConnectedDisconnected which lists the transactions which were removed from mempool due to confliction as a result of this operation. While its somewhat of a shame to make block-validation-logic generate a list of mempool changes to be included in its generated callbacks, fixing this isnt too hard.’ @morcos - you attempted something similar in #9240
  3. fanquake added the label Validation on Nov 14, 2019
  4. jnewbery force-pushed on Nov 14, 2019
  5. DrahtBot commented at 7:16 pm on November 14, 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:

    • #18278 (interfaces: Describe and follow some code conventions by ryanofsky)
    • #17443 (Drop checkFinalTx and use Median Time Past to check finality of wallet transactions by ariard)

    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.

  6. jnewbery commented at 7:27 pm on November 14, 2019: member

    Note that this can change the order of notifications for the wallet. If the node connects block A with conflicted txs a1 a2 and block B with conflicted txs b1 and b2 in one ActivateBestChainStep, the old ordering would be:

    • BlockConnected(A(conflicted=[a1,a2]))
    • BlockConnected(B(conflivted=[b1,b2]))

    and the new ordering is:

    • TransactionRemovedFromMempool(a1)
    • TransactionRemovedFromMempool(a2)
    • TransactionRemovedFromMempool(b1)
    • TransactionRemovedFromMempool(b2)
    • BlockConnected(A)
    • BlockConnected(B)

    I’m pretty sure that’s not a problem. Take a look at how the wallet currently handles conflicted transactions in the BlockConnected() callback:

    https://github.com/bitcoin/bitcoin/blob/cd6cb9745e13a62e130b11f78a13bcc1d424b05e/src/wallet/wallet.cpp#L1084

    All the TransactionRemovedFromMempool does is toggle the fInMempool flag to false.

  7. MarcoFalke commented at 7:39 pm on November 14, 2019: member

    I’m pretty sure that’s not a problem

    Are you sure about that? Note that change that is unconfirmed and not in the mempool is not counted toward our balance, so this would open up race conditions where a send* might fail when there is no reason for it to fail.

  8. jnewbery commented at 2:38 am on November 15, 2019: member

    this would open up race conditions where a send* might fail when there is no reason for it to fail.

    I don’t think that’s true:

    • Before this change: conflicted transactions are marked as not in the mempool when the wallet hears about them in a BlockConnected’s vtxConflicted vector.
    • After this change: conflicted transactions are marked as not in the mempool slightly earlier when the wallet hears about them in a TransactionRemovedFromMempool callback.

    In both of those cases, the change output is unspendable because it conflicts with a transaction included in the block, but in the current code the wallet doesn’t know about it until slightly later. So arguably there’s a window condition in the existing code where the wallet might try to spend change that it thinks is in the mempool, but is actually conflicted with the latest block (although in practice this doesn’t matter - it’s always the case that there might be a better block that conflicts our transactions that we just don’t know about yet).

    (If this PR was changing the way we notify the wallet about transactions removed from the mempool because they’re included in a block, then I’d agree with you. We don’t want to introduce a window where the wallet knows the transaction is no longer in the mempool but doesn’t yet know that it’s in a block.)

  9. MarcoFalke commented at 1:29 pm on November 15, 2019: member

    So arguably there’s a window condition in the existing code where the wallet might try to spend change that it thinks is in the mempool, but is actually conflicted with the latest block (although in practice this doesn’t matter - it’s always the case that there might be a better block that conflicts our transactions that we just don’t know about yet).

    What if the user set min_conf to 1000 and rbf to false for that tx that spends the change that is only available for a short time erroneously? With those settings, the user should expect to create a transaction that could never possibly fail, no?

  10. jnewbery commented at 1:51 pm on November 15, 2019: member

    What if the user set min_conf to 1000 and rbf to false for that tx that spends the change that is only available for a short time erroneously? With those settings, the user should expect to create a transaction that could never possibly fail, no?

    Maybe I don’t fully understand the scenario you’re describing, but if the tx is spending change that has been conflicted out of the mempool but the wallet has not yet been notified then that transaction will definitely fail, since it’s spending an output that doesn’t exist in the main chain.

    Essentially, the difference for the wallet in this PR is that it’ll get notified slightly earlier about transactions (and therefore possibly unconfirmed change outputs that it owns) that have been conflicted out of the mempool slightly before it is notified about the block that conflicts them. I can’t think of any way that would cause windows of undesirable behaviour.

  11. in src/validation.cpp:2486 in 0f17fd9bd5 outdated 
    2490  * it away and make a new one.
2491  */
2492 class ConnectTrace {
2493 private:
2494     std::vector<PerBlockConnectTrace> blocksConnected;
2495     CTxMemPool &pool;
    ariard commented at 5:45 pm on November 15, 2019:
    Commit 0f17fd9. I think after this one CTxMemPool member is no more of any use as it was used by ConnecTrace to connect to mempool signal, can you remove it ?
    jnewbery commented at 8:37 pm on November 15, 2019:
    done
  12. in src/validationinterface.h:100 in ef6f6ae651 outdated 
     99+     * This notification fires for transactions that are removed from the
100+     * mempool for the following reasons:
101+     *
102+     * - EXPIRY (expired from mempool after -mempoolexpiry hours)
103+     * - SIZELIMIT (removed in size limiting if the mempool exceeds -maxmempool megabytes)
104+     * - REORG (removed during a reorg)
    ariard commented at 6:00 pm on November 15, 2019:
    nit: “removed txn non-final anymore due to a reorg”
    jnewbery commented at 8:20 pm on November 15, 2019:

    There are several other reasons a tx could be removed for REORG:

    • a spend from a coinbase output that is no longer mature (>100 confirmations)
    • a descendants of non-final and non-mature outputs.
    • if the re-org has been deep enough that the disconnect pool has filled up
    • if the standardness or consensus rules have changed across the reorg

    I’d rather leave this high level than try to enumerate all the possible reasons for this here.

  13. in src/validation.cpp:2523 in 0f17fd9bd5 outdated 
    2483- *
2484- * This class assumes (and asserts) that the conflicted transactions for a given
2485- * block are added via mempool callbacks prior to the BlockConnected() associated
2486- * with those transactions. If any transactions are marked conflicted, it is
2487- * assumed that an associated block will always be added.
2488- *
    ariard commented at 6:19 pm on November 15, 2019:
    Commit 0f17fd9. Shoulnd’t be more logic to remove both comments in previous commit (141f971) as it’s the one where you effectively removed conflicted txn insertion and struct, Just looking at this commit atomically you can’t make sense of “This class assumes (and asserts) that the conflicted transactions for a given block are added via mempool callbacks”
    jnewbery commented at 8:23 pm on November 15, 2019:
    done
  14. in src/init.cpp:77 in f959e4a003 outdated 
    69@@ -70,6 +70,7 @@
70 #include <boost/algorithm/string/replace.hpp>
71 #include <boost/algorithm/string/split.hpp>
72 #include <boost/thread.hpp>
73+#include <boost/signals2/signal.hpp>
    ariard commented at 6:24 pm on November 15, 2019:
    super-nit: signal before thread?
    jnewbery commented at 8:23 pm on November 15, 2019:
    done
  15. in src/txmempool.h:702 in f959e4a003 outdated 
    696@@ -698,9 +697,6 @@ class CTxMemPool
697 
698     size_t DynamicMemoryUsage() const;
699 
700-    boost::signals2::signal<void (CTransactionRef)> NotifyEntryAdded;
    ariard commented at 6:27 pm on November 15, 2019:
    I think it can be noted in commit message than NotifiyEntryAdded signal didn’t have any slot connected to and so was already useless
    jnewbery commented at 8:23 pm on November 15, 2019:
    done
  16. in src/validation.cpp:2555 in 793b45c39a outdated 
    2504-        // blocks are added after all the conflicted transactions have
2505-        // been filled in. Thus, the last entry should always be an empty
2506-        // one waiting for the transactions from the next block. We pop
2507-        // the last entry here to make sure the list we return is sane.
2508-        assert(!blocksConnected.back().pindex);
2509-        blocksConnected.pop_back();
    ariard commented at 6:39 pm on November 15, 2019:
    Commit 793b45c. I think this comment, assert and pop should also be removed in commit 141f971 as this is the last commit were their logic make sense. Also in BlockConnected, the removed assert and emplace_back belonged to same logic.
    jnewbery commented at 8:24 pm on November 15, 2019:
    I prefer to leave this as a separate commit since I think it’s easier to review removing the logic piece-by-piece. If other reviewers agree with you, then I’m happy to squash the commits.
  17. in src/validation.cpp:2524 in 906b478640 outdated 
    2475-};
2476 /**
2477- * Used to track blocks whose transactions were applied to the UTXO state as a
2478- * part of a single ActivateBestChainStep call.
2479- *
2480- * This class is single-use, once you call GetBlocksConnected() you have to throw
    ariard commented at 6:41 pm on November 15, 2019:
    Should you keep this comment ? The class is still single-use.
    jnewbery commented at 8:25 pm on November 15, 2019:
    restored
  18. ariard commented at 8:01 pm on November 15, 2019: member

    Concept ACK.

    I’ve tried to implement most of my comments here : https://github.com/ariard/bitcoin/commits/2019-11-review-17477, I think it’s possible to remove the whole ConnectTrace and replace by a simple typedef’ed vector.

    What if the user set min_conf to 1000 and rbf to false for that tx that spends the change that is only available for a short time erroneously?

    Your scenario is non-rbf tx B is spending change output from non-rbf confirmed tx A but tx B is finally conflicted and so your concern is about the window time between the conflict being connected in the validation logic and the wallet getting the info to act on it ? I think this change make it better as the window time is going to be shorted due to TransactionRemovedFromMempool triggered before BlockConnected.

  19. jnewbery force-pushed on Nov 15, 2019
  20. jnewbery commented at 9:29 pm on November 15, 2019: member
    @ariard - thanks for the review. I’ve taken all of your comment suggestions and some of the changes in your branch. I’ve gone back on changing the PerBlockConnectTrace to a std::pair. Using auto and std::pair’s first and second members makes it difficult to work out what’s being done at the call site, so I’ve just changed PerBlockConnectTrace to be a struct with two members and ConnectTrace to be a std::vector. Take a look and let me know what you think.
  21. jnewbery commented at 5:59 pm on November 18, 2019: member

    @ariard : I think it’s possible to actually remove ConnectTrace entirely now that it’s not necessary to release cs_main before firing validation interface signals. Take a look at the branch here: https://github.com/jnewbery/bitcoin/tree/2019-11-remove-connect-trace.

    If you agree, I’ll remove the last two commits from this PR, and then just remove ConnectTrace entirely in a follow-up PR.

  22. jnewbery force-pushed on Nov 18, 2019
  23. DrahtBot added the label Needs rebase on Nov 21, 2019
  24. jnewbery force-pushed on Nov 22, 2019
  25. jnewbery commented at 5:04 pm on November 22, 2019: member
    rebased on master
  26. DrahtBot removed the label Needs rebase on Nov 22, 2019
  27. jnewbery force-pushed on Nov 22, 2019
  28. jnewbery commented at 7:23 pm on November 22, 2019: member
    I’ve removed the final two commits (which tidied up the ConnectTrace class) since I remove that class entirely in PR #17562.
  29. ariard commented at 8:18 pm on November 25, 2019: member
    @jnewbery - your approach is better than the one tried in my aforementioned branch, because you avoid passing back and forth a std::vector between ActivateBestChain, ActivateBestChainStep, ConnectTip. Furthermore, it’s pretty straightforward on the lock reasoning, given we already do this for BlockDisconnected in DisconnectTip where we hold cs_main
  30. in src/validation.cpp:2508 in a352662677 outdated 
    2467@@ -2468,35 +2468,21 @@ static int64_t nTimePostConnect = 0;
2468 struct PerBlockConnectTrace {
2469     CBlockIndex* pindex = nullptr;
2470     std::shared_ptr<const CBlock> pblock;
2471-    std::shared_ptr<std::vector<CTransactionRef>> conflictedTxs;
2472-    PerBlockConnectTrace() : conflictedTxs(std::make_shared<std::vector<CTransactionRef>>()) {}
2473+    PerBlockConnectTrace() {}
    promag commented at 10:47 pm on November 25, 2019:
    nit, just drop the constructor?
    jnewbery commented at 11:49 pm on November 25, 2019:
    I plan to remove PerBlockConnectTrace() entirely in #17562.
    promag commented at 1:00 pm on November 26, 2019:
    Sure, but if you happen to push again I still think you could remove this line.
  31. jnewbery force-pushed on Nov 25, 2019
  32. jnewbery commented at 11:26 pm on November 25, 2019: member
    Force-pushed a fix for a typo in a commit message. No code change.
  33. promag commented at 11:29 pm on November 25, 2019: member

    Nice cleanup, concept ACK.

    Looks like the first commit is the most sensible. Can’t CWallet::ResendWalletTransactions -> CWalletTx::SubmitMemoryPoolAndRelay mess if called in between TransactionRemovedFromMempool and BlockConnected?

  34. jnewbery commented at 11:52 pm on November 25, 2019: member

    Can’t CWallet::ResendWalletTransactions -> CWalletTx::SubmitMemoryPoolAndRelay mess if called in between TransactionRemovedFromMempool and BlockConnected?

    I’m not sure I fully understand the scenario. Can you explain further?

  35. promag commented at 1:17 pm on November 26, 2019: member

    In master CWallet::BlockConnected is “atomic” - adds new transactions and updates transaction states and nothing can can “use” the wallet in between.

    With this change “lots of stuff” can happen while the wallet state (transactions state really) is updated. So I was checking what can happen and one is: https://github.com/bitcoin/bitcoin/blob/0ee914ba9e5960763c7bb380b566ee481446f97a/src/wallet/wallet.cpp#L1747-L1748

    So it looks like a transaction can be broadcast again if this runs between TransactionRemovedFromMempool and BlockConnected. But then I’ve realised that BroadcastTransaction checks this first: https://github.com/bitcoin/bitcoin/blob/0ee914ba9e5960763c7bb380b566ee481446f97a/src/node/transaction.cpp#L28-L38

    So I think there’s no issue in this case.

  36. jnewbery commented at 2:15 pm on November 26, 2019: member

    So I think there’s no issue in this case.

    I agree, and I think this is similar to Marco’s concern earlier and my response: #17477 (comment).

    This PR allows the wallet to be used in a small window between being notified of conflicted transactions from the block and the block itself. However, I don’t think it opens any new window conditions in the wallet that don’t already exist. Imagine the following scenario in master branch:

    1. a wallet’s transaction is removed from the mempool for expiry or size limiting reasons. The wallet is notified via TransactionRemovedFromMempool
    2. a new block gets mined that contains a transaction that conflicted with the removed transaction and the BlockConnected callback is added to the Validation Interface queue (but not fired in the wallet yet)
    3. some action happens in the wallet before the BlockConnected callback is fired.

    This is exactly the same behaviour as you describe above, and can already happen in the existing code. The only difference is the mempool removal reason, which is not reported to the wallet.

  37. DrahtBot added the label Needs rebase on Jan 9, 2020
  38. jnewbery commented at 6:25 pm on January 10, 2020: member
    rebased
  39. jnewbery force-pushed on Jan 10, 2020
  40. DrahtBot removed the label Needs rebase on Jan 10, 2020
  41. in src/init.cpp:77 in 100d5d03f7 outdated 
    69@@ -70,6 +70,7 @@
70 #include <boost/algorithm/string/classification.hpp>
71 #include <boost/algorithm/string/replace.hpp>
72 #include <boost/algorithm/string/split.hpp>
73+#include <boost/signals2/signal.hpp>
    jonatack commented at 3:06 pm on January 22, 2020:
    Would this line be better in src/ui_interface.h instead? It seems to be where it’s used. Build and tests green.
    jnewbery commented at 10:13 pm on March 3, 2020:
    it’s used in init.cpp:366: static boost::signals2::connection rpc_notify_block_change_connection;
    jonatack commented at 6:12 am on March 12, 2020:
    d’oh, thanks – not sure how I missed that.
  42. jonatack commented at 3:40 pm on January 22, 2020: member

    ACK 100d5d03f7629e0c4be14239c6c6c3e542c3f34e code review, built, ran tests, ran bitcoind with mempool/validation logging, with the following reserves and to-do:

    On the potential issues raised in the discussion above, these changes appear to reduce the window but I have not yet looked through all the existing test coverage to see if more relevant coverage can be added. At first glance there seems to be a fair amount on the topic in these testfiles but it would good to dig in deeper:

    0    git grep "double spend\|double-spend\|conflicted" -- 'test' -- 'src/test/*'
  43. fjahr commented at 0:19 am on January 23, 2020: member

    Code review ACK 100d5d03f7629e0c4be14239c6c6c3e542c3f34e

    I spent some time thinking about the “window issue” and the worst scenario I could come up with is if there are high fees, a full mempool, and we do some automatic RBF-ing, so we replace a tx with a higher fee one immediately if our original is evicted from the Mempool because of low fees. We might try to RBF again although it is already in the block. That’s the only scenario I could come up with and it is an edge case with the only bad effect of sending out an RBF tx that gets rejected from every mempool. And fairly easy to prevent on the user side as well. Additionally, developers will be mindful of the window when implementing further changes, but I think that is also not a real argument against the change as it is simplifying things overall.

  44. jonatack commented at 2:56 pm on January 23, 2020: member

    FWIW if anyone is looking at tests, I reverted this change and built

    0--- a/src/txmempool.cpp
1+++ b/src/txmempool.cpp
2@@ -403,7 +403,7 @@ void CTxMemPool::addUnchecked(const CTxMemPoolEntry &entry, setEntries &setAnces
3 
4 void CTxMemPool::removeUnchecked(txiter it, MemPoolRemovalReason reason)
5 {
6-    if (reason != MemPoolRemovalReason::BLOCK) {
7+    if (reason != MemPoolRemovalReason::BLOCK && reason != MemPoolRemovalReason::CONFLICT) {
8         GetMainSignals().TransactionRemovedFromMempool(it->GetSharedTx());
9     }

    and all the unit and functional tests pass except

     02020-01-23T14:46:58.142000Z TestFramework (INFO): Initializing test directory /dev/shm/bitcoin_func_test_rjrhubgl
 12020-01-23T14:46:59.277000Z TestFramework (INFO): Mining blocks...
 22020-01-23T14:47:04.035000Z TestFramework (INFO): Running tests
 32020-01-23T14:47:16.692000Z TestFramework (ERROR): JSONRPC error
 4Traceback (most recent call last):
 5  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 112, in main
 6    self.run_test()
 7  File "test/functional/wallet_bumpfee.py", line 84, in run_test
 8    test_unconfirmed_not_spendable(rbf_node, rbf_node_address)
 9  File "test/functional/wallet_bumpfee.py", line 405, in test_unconfirmed_not_spendable
10    rbf_node.abandontransaction(bumpid)
11  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/coverage.py", line 47, in __call__
12    return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
13  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/authproxy.py", line 141, in __call__
14    raise JSONRPCException(response['error'], status)
15test_framework.authproxy.JSONRPCException: Transaction not eligible for abandonment (-5)
  45. in src/txmempool.cpp:408 in 3529e0bc2c outdated 
    405@@ -406,7 +406,7 @@ void CTxMemPool::removeUnchecked(txiter it, MemPoolRemovalReason reason)
406 {
407     CTransactionRef ptx = it->GetSharedTx();
408     NotifyEntryRemoved(ptx, reason);
409-    if (reason != MemPoolRemovalReason::BLOCK && reason != MemPoolRemovalReason::CONFLICT) {
410+    if (reason != MemPoolRemovalReason::BLOCK) {
    ryanofsky commented at 4:30 pm on January 23, 2020:

    In commit “[wallet] Notify conflicted transactions in TransactionRemovedFromMempool” (3529e0bc2cba7a9d502de2c0fe9b17ec94a7fd0c)

    Could you add a comment about why notifications are skipped for BLOCK? I believe it’s just for performance because BlockConnected notification provides the same information.

    jnewbery commented at 10:33 pm on March 3, 2020:
    done
  46. ryanofsky approved
  47. ryanofsky commented at 4:31 pm on January 23, 2020: member

    Review club notes at https://bitcoincore.reviews/17477.html

    Code review ACK 100d5d03f7629e0c4be14239c6c6c3e542c3f34e

    I’m struggling with the PR description, though. The PR description makes it sound like the reason the wallet was receiving conflicted transaction notifications through the BlockConnected signal instead of the TransactionRemovedFromMempool signal was about performance, because signals were synchronous at the time these notifications started being sent in #9371 (Jan 2017), and only later made asynchronous in #10179 (Jul 2017).

    But I think the actual reason was that:

    • The TransactionRemovedFromMempool callback didn’t exist yet. It was added in #10286 (Nov 2017)
    • #9371 wasn’t really trying to mark conflicted transactions as not being in the mempool. The CWalletTx::fInMempool boolean didn’t exist until #10286. #9371 was actually trying to fix missing -walletnotify events for these transactions. Since we stopped sending these notifications in #16624 (Sep 2019 commit 7e89994133725125dddbfa8d45484e3b9ed51c6e), it means this PR should be safe, not just because CWalletTx::fInMempool is still being kept up to date, but also because the original reason for sending these notifications is no longer there
  48. DrahtBot added the label Needs rebase on Jan 31, 2020
  49. jonatack commented at 7:23 pm on February 19, 2020: member
    Rebase, please :)
  50. jnewbery force-pushed on Feb 28, 2020
  51. DrahtBot removed the label Needs rebase on Feb 28, 2020
  52. ryanofsky approved
  53. ryanofsky commented at 6:17 pm on March 2, 2020: member

    Code review ACK b75fada45e99dac388e9501b76eba6fe7191520f. No changes since last review other than rebase.

    It would still be nice to clean up the PR description: #17477#issue-341061335 as suggested #17477#pullrequestreview-347437447, to be clearer the BlockConnected conflicted transaction list was originally intended to trigger -walletnotify notifications, not to unset fInMempool. Existing PR description seems misleading:

    These boost signals were added in #9371, before we had an asynchronous validation interface. The NotifyEntryAdded callback was used by validation to build a vector of conflicted transactions when connecting a block, which the wallet was notified of in the BlockConnected CValidationInterface callback.

    Now that we have an asynchronous TransactionRemovedFromMempool callback, we can fire that signal directly from the mempool for conflicted transactions without having to worry about the performance impact of sync’ing wallet transactions during block connection.

    I also don’t know if it was right to stop sending -walletnotify notifications in #16624, but I guess that ship has sailed, and we can always add back deleted code if needed

  54. in src/validationinterface.h:104 in b75fada45e outdated 
    103+     * - SIZELIMIT (removed in size limiting if the mempool exceeds -maxmempool megabytes)
104+     * - REORG (removed during a reorg)
105+     * - CONFLICT (removed because it conflicts with in-block transaction)
106+     * - REPLACED (removed due to RBF replacement)
107+     *
108+     * This does not fire for transactions that are removed from the mempool
    ryanofsky commented at 6:25 pm on March 2, 2020:

    In commit “[wallet] Notify conflicted transactions in TransactionRemovedFromMempool” (72d17a293e32bcf8d4cf9b7744bb67a0a886fb42)

    It’d be good to add information here from #17477 (comment) about order of TransactionRemovedFromMempool and BlockConnected notifications

    jnewbery commented at 10:39 pm on March 11, 2020:
    done
  55. jnewbery force-pushed on Mar 3, 2020
  56. jnewbery commented at 10:34 pm on March 3, 2020: member

    Pushed a fix to #17477 (review).

    Thanks for the re-review @ryanofsky . I intend to address your other comments soon.

  57. kallewoof commented at 2:26 am on March 11, 2020: member
    Code review ACK 931755508778610211b7c1cbe4952fad201e17a3
  58. [wallet] Notify conflicted transactions in TransactionRemovedFromMempool 
    The only CValidationInterface client that cares about transactions that
are removed from the mempool because of CONFLICT is the wallet.

Start using the TransactionRemovedFromMempool method to notify about
conflicted transactions instead of using the vtxConflicted vector in
BlockConnected.
    1168394d75
  59. [validation interface] Remove vtxConflicted from BlockConnected 
    The wallet now uses TransactionRemovedFromMempool to be notified about
conflicted wallet, and no other clients use vtxConflicted.
    cdb893443c
  60. [validation] Remove conflictedTxs from PerBlockConnectTrace 
    Since we don't add a vtxConflicted vector to BlockConnected the
conflictedTxs member of PerBlockConnectTrace is no longer used.
    5613f9842b
  61. [validation] Remove NotifyEntryRemoved callback from ConnectTrace 
    ConnectTrace used to subscribe to the mempool's NotifyEntryRemoved
callback to be notified of transactions removed for conflict. Since
PerBlockConnectTrace no longer tracks conflicted transactions,
ConnectTrace no longer requires these notifications.
    969b65f3f5
  62. [validation] Remove pool member from ConnectTrace 
    It's no longer used for anything.
    2dd561f361
  63. [mempool] Remove NotifyEntryAdded and NotifyEntryRemoved callbacks 
    NotifyEntryAdded never had any subscribers so can be removed.

Since ConnectTrace no longer subscribes to NotifyEntryRemoved, there are
now no subscribers.

The CValidationInterface TransactionAddedToMempool and
TransactionRemovedFromMempool methods can now provide this
functionality. There's no need for a special notifications framework for
the mempool.
    e57980b473
  64. jnewbery force-pushed on Mar 11, 2020
  65. jnewbery commented at 10:40 pm on March 11, 2020: member

    It would still be nice to clean up the PR description @ryanofsky - done. I’ve also addressed your inline comment. Let me know what you think. @kallewoof @fjahr : do you mind rereviewing? It’s just a rebase/comment change since your previous ACKs.

  66. jonatack commented at 7:43 am on March 12, 2020: member

    Re-ACK e57980b

    Retested this time after rebase on current master @ 9cc7eba, built, ran tests, ran bitcoind. Only change since last review is additional documentation: git diff b75fada..e57980b

  67. ryanofsky approved
  68. ryanofsky commented at 9:13 pm on March 12, 2020: member
    Code review ACK e57980b4738c10344baf136de3e050a3cb958ca5, no code changes since previous review, but helpful new code comments have been added and the PR description is now more clear about where the old code came from
  69. laanwj merged this on Mar 19, 2020
  70. laanwj closed this on Mar 19, 2020
  71. sidhujag referenced this in commit 3df361f60b on Mar 19, 2020
  72. domob1812 referenced this in commit b1294f7551 on Mar 23, 2020
  73. sidhujag referenced this in commit 90ffd16ccd on Nov 10, 2020
  74. deadalnix referenced this in commit b2cab2433e on Jan 11, 2021
  75. Fabcien referenced this in commit f5d5ae637e on Jan 11, 2021
  76. Fabcien referenced this in commit 03fe1825fa on Jan 11, 2021
  77. Fabcien referenced this in commit 3cf77d19ea on Jan 11, 2021
  78. Fabcien referenced this in commit 0f542ee72e on Jan 11, 2021
  79. random-zebra referenced this in commit e0350eda5b on Apr 14, 2021
  80. DrahtBot locked this on Feb 15, 2022


jnewbery DrahtBot MarcoFalke ariard promag jonatack fjahr ryanofsky kallewoof

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 15:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me