New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Mar 3, 2016

Conversation

Projects
None yet
10 participants
@morcos
Member

morcos commented Feb 11, 2016

No description provided.

@pstratem

This comment has been minimized.

Show comment
Hide comment
@pstratem

pstratem Feb 11, 2016

Contributor

utACK

5a2b1c0

Contributor

pstratem commented Feb 11, 2016

utACK

5a2b1c0

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Feb 11, 2016

Member

utACK

@pstratem Unnested ACK?

Member

sipa commented Feb 11, 2016

utACK

@pstratem Unnested ACK?

@morcos

This comment has been minimized.

Show comment
Hide comment
@morcos

morcos Feb 11, 2016

Member

@laanwj sorry, needs backport

Member

morcos commented Feb 11, 2016

@laanwj sorry, needs backport

@pstratem

This comment has been minimized.

Show comment
Hide comment
@pstratem

pstratem Feb 12, 2016

Contributor

@sipa typo

Contributor

pstratem commented Feb 12, 2016

@sipa typo

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Feb 12, 2016

Member

Why not? How will they get relayed then?

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

Member

luke-jr commented Feb 12, 2016

Why not? How will they get relayed then?

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

@morcos

This comment has been minimized.

Show comment
Hide comment
@morcos

morcos Feb 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.

Member

morcos commented Feb 12, 2016

@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.

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Feb 12, 2016

Member

ACK (with moderate testing)

Member

gmaxwell commented Feb 12, 2016

ACK (with moderate testing)

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Feb 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.

Member

luke-jr commented Feb 12, 2016

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.

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Feb 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...

Member

luke-jr commented Feb 12, 2016

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...

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Feb 12, 2016

Member

@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.

Member

gmaxwell commented Feb 12, 2016

@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.

@morcos

This comment has been minimized.

Show comment
Hide comment
@morcos

morcos Feb 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?

Member

morcos commented Feb 12, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Feb 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...

Member

luke-jr commented Feb 12, 2016

@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...

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Feb 12, 2016

Member

@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.

Member

gmaxwell commented Feb 12, 2016

@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.

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Feb 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.

Member

luke-jr commented Feb 12, 2016

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.

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Feb 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.
Member

luke-jr commented Feb 12, 2016

@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.
@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Feb 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. :(

Member

luke-jr commented Feb 12, 2016

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. :(

@laanwj laanwj added the Wallet label Feb 12, 2016

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Feb 12, 2016

Member

@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.

Member

gmaxwell commented Feb 12, 2016

@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.

@@ -1265,7 +1265,7 @@ bool CWalletTx::RelayWalletTransaction()
assert(pwallet->GetBroadcastTransactions());
if (!IsCoinBase())
{
if (GetDepthInMainChain() == 0 && !isAbandoned()) {
if (GetDepthInMainChain() == 0 && !isAbandoned() && InMempool()) {

This comment has been minimized.

@jonasschnelli

jonasschnelli Feb 19, 2016

Member

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

@jonasschnelli

jonasschnelli Feb 19, 2016

Member

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

This comment has been minimized.

@laanwj

laanwj Mar 3, 2016

Member

@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.

@laanwj

laanwj Mar 3, 2016

Member

@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.

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Feb 23, 2016

Contributor

utACK 5a2b1c0

Contributor

dcousens commented Feb 23, 2016

utACK 5a2b1c0

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Feb 25, 2016

Member

utACK

Member

sdaftuar commented Feb 25, 2016

utACK

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Mar 3, 2016

Member

utACK 5a2b1c0

Member

laanwj commented Mar 3, 2016

utACK 5a2b1c0

@laanwj laanwj merged commit 5a2b1c0 into bitcoin:master Mar 3, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Mar 3, 2016

Merge #7521: Don't resend wallet txs that aren't in our own mempool
5a2b1c0 Don't resend wallet txs that aren't in our own mempool (Alex Morcos)

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request Apr 27, 2016

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Jun 9, 2016

Member

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

Member

MarcoFalke commented Jun 9, 2016

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

thokon00 added a commit to faircoin/faircoin that referenced this pull request Jun 28, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment