Don't resend wallet txs that aren't in our own mempool #7521
Conversation
morcos
force-pushed
the
testBeforeRelay
branch
from
68f9f1c to
5a2b1c0
Compare
|
utACK |
|
utACK @pstratem Unnested ACK? |
|
@laanwj sorry, needs backport |
|
@sipa typo |
|
Why not? How will they get relayed then? At least attempt to add it to our own mempool again before failing... |
|
@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. |
|
ACK (with moderate testing) |
That's a bug. They shouldn't be negative until a conflict is mined. |
Rebroadcasts are rare anyway, right? I don't see why locking would be an issue... |
|
@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. |
|
@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? |
|
@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. |
|
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. |
|
@morcos Can you document the rationale here better in the commit message? Maybe something like: |
|
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. :( |
|
@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. |
| if (!IsCoinBase()) | ||
| { | ||
| if (GetDepthInMainChain() == 0 && !isAbandoned()) { | ||
| if (GetDepthInMainChain() == 0 && !isAbandoned() && InMempool()) { |
There was a problem hiding this comment.
orange flag: another coupling between the wallet and the full-node.
There was a problem hiding this comment.
@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.
|
utACK 5a2b1c0 |
|
utACK |
|
utACK 5a2b1c0 |
5a2b1c0 Don't resend wallet txs that aren't in our own mempool (Alex Morcos)
Github-Pull: bitcoin#7521 Rebased-From: 5a2b1c0
|
Backported as part of #7938. Removing label 'Needs backport'. |
Github-Pull: bitcoin#7521 Rebased-From: 5a2b1c0
10 participants
No description provided.