Restore UI notifications and -walletnotify behavior for block conflicted transactions #18325

issue jnewbery openend this issue on March 11, 2020
  1. jnewbery commented at 10:46 pm on March 11, 2020: member

    UI notifications and the -walletnotify process should be triggered for wallet transactions that are conflicted out of the mempool by a newly connected block.

    We broke this in #9240, reported it in #9479 and fixed it in #9371.

    We inadvertently broke it again in #16624 (Sep 2019 commit 7e89994) by removing the call to SyncTransaction() for transactions that are conflicted out of the mempool when a block is connected.

    I think the fix is just to restore the call to SyncTransaction() for transactions that are conflicted out of the mempool. Could we also call SyncTransaction() for any transaction that is removed from the mempool (and fires TransactionRemovedFromMempool)?

    h/t to @ryanofsky for spotting this. @ariard - any thoughts about just reverting 7e89994?

  2. jnewbery added the label Bug on Mar 11, 2020
  3. ryanofsky commented at 9:20 pm on March 12, 2020: member

    @ariard - any thoughts about just reverting 7e89994?

    I guess this isn’t possible if we merge #17477, but as the PR description suggests, “Now that we have a TransactionRemovedFromMempool callback”, maybe we would call SyncTransaction from there

  4. jnewbery commented at 9:32 pm on March 12, 2020: member

    maybe we would call SyncTransaction from there

    Yes, but probably just for CONFLICT transactions, to match previous behavior. We’ve never notified for transactions removed from the mempool for expiry, replacement, etc.

  5. ariard commented at 1:22 am on March 16, 2020: member
    @jnewbery if I open a new PR to revert what do you think will get merged first between it and #17477+new commit calling SyncTransaction ? I’ll add the test long-time promised for this case :)
  6. MarcoFalke added this to the milestone 0.20.0 on Mar 16, 2020
  7. MarcoFalke commented at 6:28 pm on March 16, 2020: member

    Instead of breaking this every 4 years, a test might be good to avoid it. If nobody beats me to it, I might write one.

    Also, this seems like a bugfix that should go into 0.20.0?

  8. jnewbery commented at 6:36 pm on March 17, 2020: member

    I don’t know whether #17477 can go in now that we’re in feature freeze, but as @MarcoFalke says, fixing this bug could still get in for 0.20.

    I’m happy to rebase 17477 on a fix for this.

    Instead of breaking this every 4 years, a test might be good to avoid it. If nobody beats me to it, I might write one.

    Concept ACK!

  9. ryanofsky commented at 4:46 pm on March 31, 2020: member

    We inadvertently broke it again in #16624 (Sep 2019 commit 7e89994)

    In terms of releases, 0.19 was the first version to include that commit, so the behavior’s been broken since then and maybe gone unnoticed.

  10. ariard commented at 2:32 am on April 1, 2020: member

    Likely we need to pass the eviction reason in TransactionsRemovedFromMempool to dissociate between EXPIRY,REORG and CONFLICT on the wallet side.

    UI notifications and the -walletnotify process should be triggered for wallet transactions that are conflicted out of the mempool by a newly connected block. @jnewbery conflicts should already be notified when they concern a transaction or its children already tracked by the wallet (either spending from us, or paying us). The broken case IIRC is about a non-connected tx, unconfirmed txB is paying us from an unconfirmed txA which doesn’t involve us. If txA gets conflicted we won’t see it in BlockConnected. Or did you see another uncovered case by testing ?

  11. ryanofsky commented at 2:39 am on April 1, 2020: member

    conflicts should already be notified when they concern a transaction or its children already tracked by the wallet (either spending from us, or paying us)

    Could you explain this more? Where would this happen if TransactionsRemovedFromMempool doesn’t call SyncTransaction?

  12. ryanofsky commented at 2:43 am on April 1, 2020: member
    I guess the MarkConflicted loop in AddToWalletIfInvolvingMe could trigger this, but it doesn’t seem to currently do external notifications.
  13. jnewbery commented at 2:01 pm on April 2, 2020: member

    conflicts should already be notified when they concern a transaction or its children already tracked by the wallet (either spending from us, or paying us)

    How? UI and external process notifications happen inside AddToWallet() (here: https://github.com/bitcoin/bitcoin/blob/b83565625e32b22395e28c1965b2e42fc17f04d7/src/wallet/wallet.cpp#L844 and here: https://github.com/bitcoin/bitcoin/blob/b83565625e32b22395e28c1965b2e42fc17f04d7/src/wallet/wallet.cpp#L862). AddToWallet() doesn’t get called for conflicted transactions.

  14. ariard commented at 11:52 pm on April 11, 2020: member
    @jnewbery Ah yes I meaned CWalletTx::Confirmation should be alright thanks to the MarkConflicted but we care about UI notifications here. See #18600 for further discussion :)
  15. ryanofsky referenced this in commit 2c1c162cd5 on May 5, 2020
  16. ryanofsky referenced this in commit f963a68051 on May 13, 2020
  17. laanwj referenced this in commit 99c03728c0 on May 13, 2020
  18. sidhujag referenced this in commit a84dd1fa2e on May 14, 2020
  19. fanquake referenced this in commit 9c193e4d0a on May 14, 2020
  20. MarcoFalke added the label Wallet on May 14, 2020
  21. fanquake referenced this in commit ed0afe8c1f on May 15, 2020
  22. MarcoFalke removed this from the milestone 0.20.0 on May 15, 2020
  23. MarcoFalke added this to the milestone 0.20.1 on May 15, 2020
  24. MarcoFalke commented at 12:03 pm on May 15, 2020: member

    Sep 2019 commit 7e89994

    So this is broken in all 0.19.* releases? Given that the bugfix is still “waiting for author”, I went ahead and moved this back to 0.20.1

  25. ryanofsky referenced this in commit 261ccf730c on May 15, 2020
  26. ryanofsky commented at 1:50 pm on May 15, 2020: member

    Sep 2019 commit 7e89994

    So this is broken in all 0.19.* releases?

    Yes, behavior’s been the same since #16624. I implemented a minimal fix in #18982 to just restore previous behavior while #18600 explores better approaches.

  27. ryanofsky referenced this in commit 58ec22284e on May 15, 2020
  28. ryanofsky referenced this in commit 90118ec54f on May 15, 2020
  29. ryanofsky referenced this in commit 8242b093d3 on May 21, 2020
  30. ryanofsky referenced this in commit b604c5c8b5 on May 22, 2020
  31. MarcoFalke closed this on Jun 2, 2020
  32. MarcoFalke referenced this in commit 3657aee2d2 on Jun 2, 2020
  33. sidhujag referenced this in commit 6007f5d865 on Jun 3, 2020
  34. ComputerCraftr referenced this in commit 6178216494 on Jun 5, 2020
  35. fanquake referenced this in commit 654420d6df on Jun 9, 2020
  36. luke-jr referenced this in commit 32d773bfc1 on Jun 9, 2020
  37. ComputerCraftr referenced this in commit 48245eb566 on Jun 10, 2020
  38. ComputerCraftr referenced this in commit 077eb62f7f on Jun 10, 2020
  39. stackman27 referenced this in commit 447f036161 on Jun 26, 2020
  40. metalicjames referenced this in commit 09af10dec5 on Aug 5, 2020
  41. backpacker69 referenced this in commit 2463b02ccf on Sep 8, 2020
  42. Bushstar referenced this in commit 4770dc9d56 on Oct 21, 2020
  43. Platinumwrist referenced this in commit 05e2740d92 on Oct 25, 2020
  44. janus referenced this in commit f690d20f65 on Nov 15, 2020
  45. janus referenced this in commit 6fc95ef892 on Nov 15, 2020
  46. backpacker69 referenced this in commit 364a2d33dd on Mar 28, 2021
  47. deadalnix referenced this in commit 250a0d5b25 on Aug 30, 2021
  48. MarcoFalke locked this on Feb 15, 2022


jnewbery ryanofsky ariard MarcoFalke

Labels
Bug Wallet

Milestone
0.20.1

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