Don't resend wallet txs that aren't in our own mempool #7521

pull morcos wants to merge 1 commits into bitcoin:master from morcos:testBeforeRelay changing 1 files +1 −1
  1. morcos commented at 10:14 PM on February 11, 2016: member

    No description provided.

  2. Don't resend wallet txs that aren't in our own mempool 5a2b1c0c8b
  3. morcos force-pushed on Feb 11, 2016
  4. pstratem commented at 11:21 PM on February 11, 2016: contributor

    utACK

    5a2b1c0c8b49c5ae79946e71b9f989b33c9fcf5c

  5. sipa commented at 11:22 PM on February 11, 2016: member

    utACK @pstratem Unnested ACK?

  6. morcos commented at 11:38 PM on February 11, 2016: member

    @laanwj sorry, needs backport

  7. pstratem commented at 12:11 AM on February 12, 2016: contributor

    @sipa typo

  8. luke-jr commented at 12:54 AM on February 12, 2016: member

    Why not? How will they get relayed then?

    At least attempt to add it to our own mempool again before failing...

  9. morcos commented at 1:25 AM on February 12, 2016: member

    @luke-jr My original attempt included trying to add it to your own mempool again first, but its an unfortunate amount of cs_main locking that would be needed. Also this behavior preserves the functionality from 0.11 when txs not in your mempool had GetDepthInMainChain return less than 0.

  10. gmaxwell commented at 1:30 AM on February 12, 2016: contributor

    ACK (with moderate testing)

  11. luke-jr commented at 1:44 AM on February 12, 2016: member

    Also this behavior preserves the functionality from 0.11 when txs not in your mempool had GetDepthInMainChain return less than 0.

    That's a bug. They shouldn't be negative until a conflict is mined.

  12. luke-jr commented at 1:44 AM on February 12, 2016: member

    My original attempt included trying to add it to your own mempool again first, but its an unfortunate amount of cs_main locking that would be needed.

    Rebroadcasts are rare anyway, right? I don't see why locking would be an issue...

  13. gmaxwell commented at 1:45 AM on February 12, 2016: contributor

    @luke-jr This needs to be the simplest and most obviously-does-no-harm change to avoid a serious behavior regression in 0.12. The whole rebroadcast logic in general is brain-damaged and privacy destroying and we should fix it more completely, but this PR is not the time.

  14. morcos commented at 1:48 AM on February 12, 2016: member

    @luke-jr yes, i'm saying that in 0.11 they were negative so weren't rebroadcast. now they aren't negative, so we need to explicitly not rebroadcast them. i dont think they are rare are they?

  15. luke-jr commented at 2:31 AM on February 12, 2016: member

    @morcos It was literally impossible to have a wallet transaction that was rejected by your own mempool, in 0.11. @gmaxwell What behaviour is regressed right now? The PR is severely lacking in details on what it's trying to fix...

  16. gmaxwell commented at 2:37 AM on February 12, 2016: contributor

    @luke-jr In 0.11 same as in 0.12, a wallet transaction will be rejected by your mempool if your fee settings are higher than the tx pays.

    The regressed behavior is that in 0.12 the node will continually rebroadcast non-mempoolable (and non-mempool) transactions; which is incredibly deanonymizing (and somewhat bandwidth wasting). E.g. you pay someone a tiny amount with a 1e-8/byte fee when their mempool isn't full and then their client will continue to beacon forever... and because their own node hasn't and wouldn't mempool the transaction, it's completely clear that they're the origin.

  17. luke-jr commented at 2:49 AM on February 12, 2016: member

    Oh! So this doesn't restore the "-1 if not in mempool" bug, it only restores the side effect of that preventing rebroadcast. Makes [enough] sense for now. Concept ACK.

  18. luke-jr commented at 2:52 AM on February 12, 2016: member

    @morcos Can you document the rationale here better in the commit message? Maybe something like:

    wallet: Don't resend wallet txs that aren't in our own mempool

Rebroadcasting of wtx for not-in-mempool transactions was previously prevented by GetDepthInMainChain erroneously returning -1 for them. This restores that previous behaviour.
  19. luke-jr commented at 2:54 AM on February 12, 2016: member

    Actually, this is a new condition in 0.12, since 0.11 never would create a transaction that wouldn't be accepted to the mempool (and why the -1 bug wasn't really quite a bug in 0.11). So this does seem to indeed still change the behaviour negatively... so I guess I'm back to not seeing a justification for this change. :(

  20. laanwj added the label Wallet on Feb 12, 2016
  21. gmaxwell commented at 10:43 AM on February 12, 2016: contributor

    @luke-jr "wouldn't create" is too limited, people sendrawtransaction, wallets get copied, nodes get restarted with different settings, and this covers received transactions too.

  22. laanwj added the label Needs backport on Feb 15, 2016
  23. in src/wallet/wallet.cpp:None in 5a2b1c0c8b 
    1264 | @@ -1265,7 +1265,7 @@ bool CWalletTx::RelayWalletTransaction()
1265 |      assert(pwallet->GetBroadcastTransactions());
1266 |      if (!IsCoinBase())
1267 |      {
1268 | -        if (GetDepthInMainChain() == 0 && !isAbandoned()) {
1269 | +        if (GetDepthInMainChain() == 0 && !isAbandoned() && InMempool()) {
    jonasschnelli commented at 4:07 PM on February 19, 2016:

    orange flag: another coupling between the wallet and the full-node.

    laanwj commented at 2:08 PM on March 3, 2016:

    @jonasschnelli Sometimes I think 'outgoing' transactions need to be in a special section of the wallet. A bit like how mail programs have an 'outbox'. Newly created transactions will end up there, and are removed after trying to broadcast for a while - or when the user wants to give up on them.

    It would be better than considering all transactions for rebroadcast. At the least it would give the user explicit control over that.

    In any case, it doesn't need to block this sensible privacy improvement, but I agree that the rebroadcast logic is still far from optimal.

  24. dcousens commented at 1:11 AM on February 23, 2016: contributor

    utACK 5a2b1c0

  25. paveljanik referenced this in commit 327580d00a on Feb 23, 2016
  26. sdaftuar commented at 1:34 AM on February 25, 2016: member

    utACK

  27. laanwj commented at 2:09 PM on March 3, 2016: member

    utACK 5a2b1c0

  28. laanwj merged this on Mar 3, 2016
  29. laanwj closed this on Mar 3, 2016
  30. laanwj referenced this in commit 3368895c3b on Mar 3, 2016
  31. MarcoFalke referenced this in commit 097d7b7688 on Apr 25, 2016
  32. MarcoFalke referenced this in commit 21b2f82eb7 on Apr 27, 2016
  33. MarcoFalke commented at 10:56 AM on June 9, 2016: member

    Backported as part of #7938. Removing label 'Needs backport'.

  34. MarcoFalke removed the label Needs backport on Jun 9, 2016
  35. thokon00 referenced this in commit 8086de939d on Jun 28, 2016
  36. DrahtBot locked this on Sep 8, 2021
Contributors
morcospstratemsipaluke-jrgmaxwelljonasschnellilaanwjdcousenssdaftuarMarcoFalke
Labels
Wallet
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: 2026-04-21 15:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me