wallet: don't create long chains by default

glozow commented at 11:52 am on March 8, 2022: member

Default mempool policy doesn’t let you have chains longer than 25 transactions. This is locally configurable of course, but it’s not really safe to assume that a chain longer than 25 transactions will propagate. Thus, the wallet should probably avoid creating such transactions by default; set DEFAULT_WALLET_REJECT_LONG_CHAINS to true.

Closes #9752 Closes #10004

fanquake added the label Wallet on Mar 8, 2022

MarcoFalke commented at 12:00 pm on March 8, 2022: member

Previous discussion: https://github.com/bitcoin/bitcoin/pull/10015

glozow commented at 12:08 pm on March 8, 2022: member

Sad that #10015 never landed. fwiw not changing the error message here - I think “insufficient funds” is the right message to return if you’re unable to produce a transaction that would be accepted by your mempool.

MarcoFalke commented at 12:34 pm on March 8, 2022: member

So the change here will be:

Before a user would see their balance deplete, until they run out of funds (as their change is ignored, see #11887), then coin selection will fail. Previously created txs will likely broadcast at some point and the balance returns to normal.
Now a user would see coin selection fail, but have an accurate balance.

In both cases the user will see coin selection fail, so it seems the underlying user problem still exists. Though, it is probably fine to postpone a fix for that until after #11887, in which case this pull can be reverted again.

achow101 commented at 12:48 pm on March 8, 2022: member

Concept ACK

I don’t think it makes sense for the wallet to by default make transactions that won’t be relayed immediately. The current behavior also leads to user confusion about their balance.

I think “insufficient funds” is the right message to return if you’re unable to produce a transaction that would be accepted by your mempool.

I think “insufficient funds” is too generic of an error message for coin selection failures as there are a ton of different reasons why coin selection might fail. I would like to have more granular error messages, but changing that is orthogonal to this.

achow101 commented at 5:49 pm on March 23, 2022: member

ACK 9ca1de5127f590e723f9d14868060cdfd8b09e94

in test/functional/wallet_balance.py:55 in 9ca1de5127 outdated

49@@ -50,7 +50,9 @@ def set_test_params(self):
50         self.num_nodes = 2
51         self.setup_clean_chain = True
52         self.extra_args = [
53-            ['-limitdescendantcount=3'],  # Limit mempool descendants as a hack to have wallet txs rejected from the mempool
54+            # Limit mempool descendants as a hack to have wallet txs rejected from the mempool.
55+            # Set walletrejectlongchains=false so the wallet still creates the transactions.
56+            ['-limitdescendantcount=3', '-walletrejectlongchains=false'],

MarcoFalke commented at 6:37 am on March 24, 2022:

nit: pretty sure you can’t use boolean values here. (Otherwise the test should fail with =true, which I presume it doesn’t ?)

0            # Limit mempool descendants as a hack to have wallet txs rejected from the mempool.
1            # Set walletrejectlongchains=0 so the wallet still creates the transactions.
2            ['-limitdescendantcount=3', '-walletrejectlongchains=0'],

glozow commented at 3:13 pm on March 25, 2022:

Fixed

in test/functional/wallet_basic.py:571 in 9ca1de5127 outdated

567@@ -568,7 +568,7 @@ def run_test(self):
568         self.log.info("Test -reindex")
569         self.stop_nodes()
570         # set lower ancestor limit for later
571-        self.start_node(0, ['-reindex', "-limitancestorcount=" + str(chainlimit)])
572+        self.start_node(0, ['-reindex', "-walletrejectlongchains=false", "-limitancestorcount=" + str(chainlimit)])

MarcoFalke commented at 6:37 am on March 24, 2022:

Same?

glozow commented at 3:13 pm on March 25, 2022:

Fixed

MarcoFalke approved

MarcoFalke commented at 6:38 am on March 24, 2022: member

lgtm, but I think there are typos?

glozow force-pushed on Mar 25, 2022

in test/functional/wallet_basic.py:145 in 93b4a81ce2 outdated

141@@ -142,7 +142,7 @@ def run_test(self):
142         self.nodes[2].lockunspent(False, [unspent_0], True)
143 
144         # Restarting the node with the lock written to the wallet should keep the lock
145-        self.restart_node(2)
146+        self.restart_node(2, ["-walletrejectlongchains=false"])

MarcoFalke commented at 3:57 pm on March 25, 2022:

:eyes:

in test/functional/wallet_basic.py:28 in 93b4a81ce2 outdated

24@@ -25,7 +25,7 @@ class WalletTest(BitcoinTestFramework):
25     def set_test_params(self):
26         self.num_nodes = 4
27         self.extra_args = [[
28-            "-acceptnonstdtxn=1",
29+            "-acceptnonstdtxn=1", "-walletrejectlongchains=false"

MarcoFalke commented at 3:58 pm on March 25, 2022:

:eyes:

MarcoFalke commented at 3:58 pm on March 25, 2022: member

I’ve marked the remaining typos with :eyes:

[wallet] don't create long chains by default da2bc865d6

glozow force-pushed on Mar 25, 2022

MarcoFalke commented at 4:15 pm on March 25, 2022: member

re-ACK da2bc865d644f6be748c305556bdd02f02d1b161 only change is fixing typos in tests 🎏

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3re-ACK da2bc865d644f6be748c305556bdd02f02d1b161 only change is fixing typos in tests 🎏
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUgIugv/ddcX8zoLuze/7EuMxEnxlwIMKPYVFfzrFX2cLnCnpf+jIqwArqhmM5h8
 8yWrv6P877baXdFizgrlGQu5gMEFDPE8ZrXZKmzN1sg0dj+nc4W25zNSrfe9VP4Nw
 9AyumBUkzjznXrktn2ICNE7d3Ye2BbSLNbfcXVGmCgUq+ilwvmXoSvVU5SqUs6tlP
10/yQQUxlR0Nn6aj0ABcbPhJOdZO8JvgVA1o4hPIPzDwEqmzsDDaqnQrHlJfhRHxU9
11mTsaR9fX6zJxjdyrUdQfpP9vO2jIv9Fdv791SD3+ECIwAOsXIbSnrj3r3rhQ1DPu
12aKk45J6gFDqt9Ls4Q5rCm6lR+9Zt4PzyjW9iQR41NenLZlXJh9NeK496wkioZm0S
13+N7+nwrJQR6Z2A09KxsVKOXM95jJHmrsFGNR7K85GDeXWtm1gGjKUnImzmyvGpY/
148/cUPqXq5PSnl0vh+9CbXAF3uszowQWwo4+gXJCkd3KKjezXQWruzOieEJod2yeZ
15ADGknmuH
16=K1MH
17-----END PGP SIGNATURE-----

MarcoFalke merged this on Mar 25, 2022

MarcoFalke closed this on Mar 25, 2022

glozow deleted the branch on Mar 25, 2022

achow101 commented at 4:21 pm on March 25, 2022: member

re-ACK da2bc865d644f6be748c305556bdd02f02d1b161

t-bast commented at 5:42 pm on March 25, 2022: member

Late ACK, thanks for updating this!

We’ve been running with this parameter set to true ever since we discovered this flag, because one day we got bitten by this in production and realized this problem.

sidhujag referenced this in commit 92432625ce on Apr 2, 2022

DrahtBot locked this on Mar 25, 2023

wallet: don’t create long chains by default #24502