Make RelayWalletTransaction attempt to AcceptToMemoryPool. #9290

pull gmaxwell wants to merge 1 commits into bitcoin:master from gmaxwell:resend_retries_mempool changing 1 files +4 −2

gmaxwell commented at 6:49 AM on December 6, 2016: contributor

This resolves an issue where a wallet transaction which failed to relay previously because it couldn't make it into the mempool will not try again until restart, even though mempool conditions may have changed. Rebroadcast of other transactions is unchanged and works as it always did.

Abandoned and known-conflicted transactions are skipped.

Some concern was expressed that there may be users with many unknown conflicts would waste a lot of CPU time trying to add them to their memory pools over and over again. But I am doubtful these users exist in any number, if they do exist they have worse problems, and they can mitigate any performance issue this might have by abandoning the transactions in question.
gmaxwell cross-referenced this on Dec 6, 2016 from issue Prefer coins that have fewer ancestors, sanity check txn before ATMP by instagibbs

Make RelayWalletTransaction attempt to AcceptToMemoryPool.

This resolves an issue where a wallet transaction which failed to
 relay previously because it couldn't make it into the mempool
 will not try again until restart, even though mempool conditions
 may have changed.

Abandoned and known-conflicted transactions are skipped.

Some concern was expressed that there may be users with many
 unknown conflicts would waste a lot of CPU time trying to
 add them to their memory pools over and over again.  But I am
 doubtful these users exist in any number, if they do exist
 they have worse problems, and they can mitigate any performance
 issue this might have by abandoning the transactions in question.

f692fce8a4

in src/wallet/wallet.cpp:None in e72d48558d outdated

1547 |      {
1548 | -        if (GetDepthInMainChain() == 0 && !isAbandoned() && InMempool()) {
1549 | +        LOCK(mempool.cs);
1550 | +        CValidationState state;
1551 | +        /* GetDepthInMainChain already catches known conflicts. */
1552 | +        if (mempool.exists(GetHash()) || AcceptToMemoryPool(maxTxFee, state)) {

sipa commented at 7:13 AM on December 6, 2016:

If you use InMempool() here instead of mempool.exists(GetHash()), you can drop the LOCK(mempool.cs).

MarcoFalke commented at 10:24 AM on December 6, 2016: member

utACK f692fce8a49e05e25f1c767aae1e50db419caebe
MarcoFalke added the label Wallet on Dec 6, 2016
gmaxwell commented at 6:23 PM on December 8, 2016: contributor

If we merge this, I'm also going to recommend we backport it.
morcos commented at 6:48 PM on December 8, 2016: member

Alex was here
sdaftuar commented at 7:06 PM on December 8, 2016: member

Concept and utACK, will test.
jonasschnelli commented at 7:06 PM on December 8, 2016: contributor

utACK f692fce8a49e05e25f1c767aae1e50db419caebe
luke-jr commented at 2:38 AM on December 9, 2016: member

utACK, but I'm not clear on what "unknown conflicts" in OP refer to... shouldn't all conflicts be known?
gmaxwell commented at 3:13 PM on December 9, 2016: contributor

@luke-jr If you take an old wallet-- one from prior to the conflict tracking-- with conflicts and load it, and do not rescan the whole blockchain, then there will be conflicts which are unknown to the wallet. ATMP will fail, but the wallet won't know why.
sdaftuar commented at 3:19 PM on December 9, 2016: member

Tested this along with #9302, ACK.
rebroad commented at 12:02 PM on December 10, 2016: contributor

Is it worth notifiying the user when their tx is not being relayed? that way they might choose to bump the fee if it is desired.
gmaxwell commented at 7:39 PM on December 10, 2016: contributor

@rebroad It's logged and the listtransaction information shows its not in the mempool. More will be done later, no doubt.
MarcoFalke added this to the milestone 0.13.2 on Dec 11, 2016
MarcoFalke added the label Needs backport on Dec 11, 2016
MarcoFalke commented at 10:13 PM on December 11, 2016: member

Tested this with #9302 on current master

ACK
instagibbs commented at 8:14 PM on December 12, 2016: member

utACK f692fce8a49e05e25f1c767aae1e50db419caebe
MarcoFalke assigned laanwj on Dec 12, 2016
laanwj referenced this in commit 782328660e on Dec 14, 2016
sipa merged this on Dec 14, 2016
sipa closed this on Dec 14, 2016
sipa referenced this in commit 57e337d40e on Dec 14, 2016
MarcoFalke referenced this in commit f7b94f721c on Dec 14, 2016
MarcoFalke removed the label Needs backport on Dec 14, 2016
MarcoFalke commented at 12:09 PM on December 14, 2016: member

Backport in #9347 (release notes already merged)
MarcoFalke referenced this in commit c3eed6accc on Dec 14, 2016
MarcoFalke referenced this in commit f37e1f333f on Dec 14, 2016
MarcoFalke referenced this in commit 4291feccf5 on Dec 14, 2016
MarcoFalke referenced this in commit 35174a0280 on Dec 14, 2016
morcos cross-referenced this on Dec 15, 2016 from issue Increase mempool expiry time to 2 weeks by morcos
dexX7 referenced this in commit 2fdbc202ae on Jan 9, 2017
dexX7 referenced this in commit b1c266f414 on Jan 9, 2017
dexX7 referenced this in commit afb0c58e0b on Jan 23, 2017
jnewbery cross-referenced this on Mar 16, 2017 from issue Wallet should reject long chains by default by jnewbery
dexX7 referenced this in commit a400d26561 on Jun 8, 2017
dexX7 referenced this in commit 2476c64723 on Jun 8, 2017
dexX7 referenced this in commit 3edbe7cc79 on Jun 8, 2017
codablock referenced this in commit e6d05d71dd on Jan 18, 2018
andvgal referenced this in commit 14de1354ab on Jan 6, 2019
CryptoCentric referenced this in commit c6ab8312b1 on Feb 26, 2019
random-zebra cross-referenced this on Jun 5, 2020 from issue [Wallet] ATMP failure in CommitTransaction by random-zebra
furszy cross-referenced this on Aug 2, 2020 from issue [Wallet] Make RelayWalletTransaction attempt to AcceptToMemoryPool. by furszy
bitcoin locked this on Sep 8, 2021