rpc, wallet: fix incorrect segwit redeem script size limit

furszy commented at 11:18 pm on August 21, 2023: member

Currently, redeem scripts longer than 520 bytes, which are technically valid under segwit rules, have flaws in the following processes:

The multisig creation process fails to deduce the output descriptor, resulting in the generation of an incorrect descriptor. Additionally, the accompanying user warning is also inaccurate.
The signrawtransactionwithkey RPC command fail to sign them.
The legacy wallet addmultisigaddress wrongly discards them.

The issue arises because most of these flows are utilizing the legacy spkm keystore, which imposes the p2sh max redeem script size rule on all scripts. Which blocks segwit redeem scripts longer than the max element size in all the previously mentioned processes (createmultisig, addmultisigaddress, and signrawtransactionwithkey).

This PR fixes the problem, enabling the creation of multisig output descriptors involving more than 15 keys and allowing the signing of these scripts, along with other post-segwit redeem scripts that surpass the 520-byte p2sh limit.

Important note: Instead of adding support for these longer redeem scripts in the legacy wallet, an “unsupported operation” error has been added. The reasons behind this decision are:

The introduction of this feature brings about a compatibility-breaking change that requires downgrade protection; older wallets would be unable to interact with these “new” legacy wallets.
Considering the ongoing deprecation of the legacy spkm, this issue provides another compelling reason to transition towards descriptors.

Testing notes: To easily verify each of the fixes, I decoupled the tests into standalone commits. So they can be cherry-picked on top of master. Where rpc_createmultisig.py (with and without the --legacy-wallet arg) will fail without the bugs fixes commits.

Extra note: The initial commits improves the rpc_createmultisig.py test in many ways. I found this test very antiquated, screaming for an update and cleanup.

DrahtBot commented at 11:18 pm on August 21, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	pinheadmz, theStack, achow101

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#30019 (test: use assert_greater_than util by kevkevinpal)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label CI failed on Aug 22, 2023

furszy force-pushed on Aug 22, 2023

DrahtBot removed the label CI failed on Aug 22, 2023

glozow added the label Wallet on Sep 1, 2023

DrahtBot added the label CI failed on Sep 3, 2023

DrahtBot removed the label CI failed on Sep 5, 2023

pinheadmz commented at 4:29 pm on September 6, 2023: member

concept ACK db38b584e6

Confirmed this branch fixes the issue mentioned in #28250

Code review to follow…

0{
1  "address": "3JDRACdkEs41yxtYLi6kiqQJdQHLPDxehc",
2  "redeemScript": "602102f293c4d026a536a082453f8327f305ae0a0c59350b396205e15fbec2af67390b2103d7b6a7e972f6e1a532f9f49a7cc35055a0ffd7cec5f81728f6a3fdf4dad9367221021f360d51b8eb43422fe942c837ad16e36bab87c5f7567609de9a38e205fb51ed2103a778eaee9cc18a4e5e02a78f403082d4e6f3a9c0d9edaea20b9ad86089de7fc92102a3c95e41e8272c6842df8c9da492ef5e03bbc4ca17c22ce1f88570e716e187db21032e1d465bc5cdea674337934fb861d097dd7ab20289e4d41de14a6cafcd61d30a210370fd35da88778a805ce9acc88c0020e3b14cacaa30f99d829207e1896d782ea12103878a4b831af4fee069964448ccca999071633fcc751d296e8c5325167274febf210248f8621596029568070dda40bc1e3ace78df320b5ebf0d6a431536e9af3ec8942103f757260835b7e439b244a9b10f5645966d7dcff9d2648c400a0d6827f4a64ac421023b3972554e123985b6c3d8575e6fd3384f59a58ef91e0d39ba5905dad49e73722102e32652928f21764e02822d39ffd1075c428e372a06fe7247023d5b60f1d0159621020e002818c48b1d1766f6196e09db90f51e155bbd49be0ad743d1fa6b7e8efcae2103725d482f0c77f4feb1ecbea28f036b30889f462222002e88aa8f8346dce9447421031fa2798d9f40bc2746f91f300460be9091e10f1b4baa37843cd41c965b0f9b41210235b7b545398ce9692f319e6eefe114eae50f008f46d33baf19418e0f17e3bc652102ff72a2b54370c0b1501228b6a210bc36a16535cc358bf7839c85a6ae2c41725c210282ce44556f5a81c7a698dc8fdba12089740df420af93002e80a752020e5d939e210279f5b244862e27f69284befb0299909139b899d2702b3e74b4317ed5b2947e03210293d0b090906afdee07dabedf34798293246ce06ade2e3887302f289008f25f4d0114ae",
3  "descriptor": "sh(wsh(multi(16,02f293c4d026a536a082453f8327f305ae0a0c59350b396205e15fbec2af67390b,03d7b6a7e972f6e1a532f9f49a7cc35055a0ffd7cec5f81728f6a3fdf4dad93672,021f360d51b8eb43422fe942c837ad16e36bab87c5f7567609de9a38e205fb51ed,03a778eaee9cc18a4e5e02a78f403082d4e6f3a9c0d9edaea20b9ad86089de7fc9,02a3c95e41e8272c6842df8c9da492ef5e03bbc4ca17c22ce1f88570e716e187db,032e1d465bc5cdea674337934fb861d097dd7ab20289e4d41de14a6cafcd61d30a,0370fd35da88778a805ce9acc88c0020e3b14cacaa30f99d829207e1896d782ea1,03878a4b831af4fee069964448ccca999071633fcc751d296e8c5325167274febf,0248f8621596029568070dda40bc1e3ace78df320b5ebf0d6a431536e9af3ec894,03f757260835b7e439b244a9b10f5645966d7dcff9d2648c400a0d6827f4a64ac4,023b3972554e123985b6c3d8575e6fd3384f59a58ef91e0d39ba5905dad49e7372,02e32652928f21764e02822d39ffd1075c428e372a06fe7247023d5b60f1d01596,020e002818c48b1d1766f6196e09db90f51e155bbd49be0ad743d1fa6b7e8efcae,03725d482f0c77f4feb1ecbea28f036b30889f462222002e88aa8f8346dce94474,031fa2798d9f40bc2746f91f300460be9091e10f1b4baa37843cd41c965b0f9b41,0235b7b545398ce9692f319e6eefe114eae50f008f46d33baf19418e0f17e3bc65,02ff72a2b54370c0b1501228b6a210bc36a16535cc358bf7839c85a6ae2c41725c,0282ce44556f5a81c7a698dc8fdba12089740df420af93002e80a752020e5d939e,0279f5b244862e27f69284befb0299909139b899d2702b3e74b4317ed5b2947e03,0293d0b090906afdee07dabedf34798293246ce06ade2e3887302f289008f25f4d)))#xfjxuvgn"
4}

in src/outputtype.cpp:87 in db38b584e6 outdated

86+CTxDestination AddAndGetDestinationForScript(FlatSigningProvider& keystore, const CScript& script, OutputType type)
87 {
88     // Add script to keystore
89-    keystore.AddCScript(script);
90-    // Note that scripts over 520 bytes are not yet supported.
91+    keystore.scripts.emplace(CScriptID(script), script);

pinheadmz commented at 6:02 pm on September 6, 2023:

Just curious, why doesn’t FlatSigningProvider have a AddCScript() method like it’s Fillable counterpart? I was surprised to see the maps in the provider aren’t private or protected, I thought that was more our style.

furszy commented at 8:01 pm on September 6, 2023:

Just curious, why doesn’t FlatSigningProvider have a AddCScript() method like it’s Fillable counterpart? I was surprised to see the maps in the provider aren’t private or protected, I thought that was more our style.

FillableSigningProvider is the legacy counterpart. All other SigningProvider subclasses were introduced with the output descriptors implementation.

If you look at the new providers, all of them are loaded in place and stay constant over time. They are only accessed through the SigningProvider interface methods and not directly (this interface only present getters). In contraposition, the legacy provider fulfills both roles. It implements the SigningProvider methods and, at the same time, allows new scripts and keys to be added over time. This reflects the legacy vs descriptors difference: each new import in the descriptors wallet refers to a new spkm, while in the legacy wallet, it refers to a new script/key added into the single legacy spkm.

pinheadmz commented at 8:11 pm on September 6, 2023:

So all the places where we call FlatSigningProvider.scripts.emplace() doesn’t count as “allows new scripts and keys to be added over time” ?

furszy commented at 8:29 pm on September 6, 2023:

So all the places where we call FlatSigningProvider.scripts.emplace() doesn’t count as “allows new scripts and keys to be added over time” ?

As uses to be in software development, the answer is “it depends”. As far as I can tell, the idea was to hide the SigningProvider implementation. Which is sound.

I haven’t looked at all the places where the FlatSigningProvider is used but most of them should be inside the descriptor.cpp and similar related processes where the parent process invokes a function to load the provider with some keys/scripts, to then use them and discard the provider.

When I said “allows new scripts and keys to be added over time” was referring to keep it in-memory. And continually add new elements to it. Which should not be happening with FlatSigningProvider anywhere.

in test/functional/rpc_createmultisig.py:96 in db38b584e6 outdated

105-        assert bal1 == 0
106-        assert bal2 == self.moved
107-        assert_equal(bal0 + bal1 + bal2 + balw, total)
108+        self.log.info('Test valid 16-20 multisig p2sh-legacy and bech32 (no wallet)')
109+        self.do_multisig(nkeys=20, nsigs=16, output_type="p2sh-segwit", wallet_multi=None)
110+        self.do_multisig(nkeys=20, nsigs=16, output_type="bech32", wallet_multi=None)

pinheadmz commented at 6:07 pm on September 6, 2023:

do we need to cover m=21 here to catch the error in segwit multisig?

furszy commented at 8:02 pm on September 6, 2023:

yeah, good idea. Will add it.

furszy force-pushed on Sep 6, 2023

furszy commented at 9:02 pm on September 6, 2023: member

Updated per feedback. Thanks @pinheadmz. Small diff .

Added coverage for createmultisig +20 keys error.

DrahtBot added the label CI failed on Jan 12, 2024

in test/functional/test_framework/test_framework.py:442 in 8c3fdc4bbf outdated

436@@ -437,6 +437,11 @@ def init_wallet(self, *, node):
437                 n.createwallet(wallet_name=wallet_name, descriptors=self.options.descriptors, load_on_startup=True)
438             n.importprivkey(privkey=n.get_deterministic_priv_key().key, label='coinbase', rescan=True)
439 
440+    # Only enables wallet when the module is available
441+    def enable_wallet_if_possible(self):
442+        self._requires_wallet = self.is_wallet_compiled() and (self.is_sqlite_compiled() or self.is_bdb_compiled())

theStack commented at 11:04 am on January 26, 2024:

IIUC, the right part of this expression seems redundant?

0        self._requires_wallet = self.is_wallet_compiled()

If a wallet is compiled, that implies that either SQLite or BDB is compiled as well.

Consequently, the self.has_wallet member is not needed anymore, and can simply be replaced by self.is_wallet_compiled() calls?

furszy commented at 1:54 pm on January 26, 2024:

If a wallet is compiled, that implies that either SQLite or BDB is compiled as well.

Yeah sure. Removed the right part of the statement.

Consequently, the self.has_wallet member is not needed anymore, and can simply be replaced by self.is_wallet_compiled() calls?

Sure. Done.

theStack commented at 1:29 pm on January 26, 2024: contributor

Concept ACK

furszy force-pushed on Jan 26, 2024

furszy commented at 1:57 pm on January 26, 2024: member

Updated per feedback. Thanks theStack.

DrahtBot removed the label CI failed on Jan 26, 2024

in src/wallet/rpc/addresses.cpp:303 in 3997791eba outdated

289@@ -290,7 +290,20 @@ RPCHelpMan addmultisigaddress()
290 
291     // Import scripts into the wallet
292     for (const auto& [id, script] : provider.scripts) {
293-        spk_man.AddCScript(script);
294+        // Due to a bug in the legacy wallet, the p2sh maximum script size limit is also imposed on 'p2sh-segwit' and 'bech32' redeem scripts.
295+        // Even when redeem scripts over MAX_SCRIPT_ELEMENT_SIZE bytes are valid for segwit output types, we don't want to
296+        // enable it because:
297+        // 1) It introduces a compatibility-breaking change requiring downgrade protection; older wallets would be unable to interact with these "new" legacy wallets.
298+        // 2) Considering the ongoing deprecation of the legacy spkm, this issue adds another good reason to transition towards descriptors.

rkrux commented at 11:01 am on March 31, 2024:

Is it correct to assume that in the future releases by getting rid of the legacy spkm, we will get rid of FillableSigningProvider as well?

furszy commented at 2:45 pm on April 1, 2024:

Is it correct to assume that in the future releases by getting rid of the legacy spkm, we will get rid of FillableSigningProvider as well?

Yes.

in test/functional/rpc_createmultisig.py:151 in 6df7d2ce5a outdated

144-        assert 150 < height < 350
145-        total = 149 * 50 + (height - 149 - 100) * 25
146-        assert bal1 == 0
147-        assert bal2 == self.moved
148-        assert_equal(bal0 + bal1 + bal2 + balw, total)
149-

rkrux commented at 11:06 am on March 31, 2024:

+1 on removing these hard-coded values!

in test/functional/rpc_createmultisig.py:222 in fffa9bb9a0 outdated

200+                result = wallet_multi.addmultisigaddress(2, keys, '', 'legacy')
201+                assert_equal(legacy_addr, result['address'])
202+                assert 'warnings' not in result
203+
204+            # Generate addresses with the segwit types. These should all make legacy addresses
205+            err_msg = ["Unable to make chosen address type, please ensure no uncompressed public keys are present."]

rkrux commented at 11:08 am on March 31, 2024:

Nit: err_msg_list

furszy commented at 2:22 pm on April 27, 2024:

sure, will change it if I have to retouch the code.

in test/functional/rpc_createmultisig.py:61 in 29e7bb80cb outdated

61-            for nsigs in [2, 3]:
62-                for output_type in ["bech32", "p2sh-segwit", "legacy"]:
63-                    self.do_multisig(nkeys, nsigs, output_type, wallet_multi)
64+        self.create_keys(21)  # max number of allowed keys + 1
65+        m_of_n = [(2, 3), (3, 3), (2, 5), (3, 5), (10, 15), (15, 15)]
66+        for (sigs, keys) in m_of_n:

rkrux commented at 11:10 am on March 31, 2024:

+1 on creating a list of (sigs, keys) instead!

theStack commented at 3:51 pm on May 17, 2024:

nit (feel free to ignore): imho keeping the nsigs/nkeys naming here makes sense, to make it clearer that these are counts, not actual signatures and keys

in test/functional/rpc_createmultisig.py:100 in 29e7bb80cb outdated

 95+        self.do_multisig(nkeys=20, nsigs=16, output_type="p2sh-segwit", wallet_multi=None)
 96+        self.do_multisig(nkeys=20, nsigs=16, output_type="bech32", wallet_multi=None)
 97+
 98+        self.log.info('Test invalid 16-21 multisig p2sh-legacy and bech32 (no wallet)')
 99+        assert_raises_rpc_error(-8, "Number of keys involved in the multisignature address creation > 20", node1.createmultisig, 16, self.pub, 'p2sh-segwit')
100+        assert_raises_rpc_error(-8, "Number of keys involved in the multisignature address creation > 20", node1.createmultisig, 16, self.pub, 'bech32')

rkrux commented at 11:12 am on March 31, 2024:

0for addr_type in ['p2sh-segwit', 'bech32']:
1 assert_raises_rpc_error(-8, "Number of keys involved in the multisignature address creation > 20", node1.createmultisig, 16, self.pub, addr_type)

furszy commented at 2:23 pm on April 27, 2024:

sure, will change it if I have to retouch this code.

in test/functional/rpc_createmultisig.py:111 in a0ebb929e8 outdated

132+
133+            self.log.info('Test legacy wallet unsupported operation. 16-20 multisig p2sh-legacy and bech32 generation')
134+            # Due an internal limitation on legacy wallets, the redeem script limit also applies to p2sh-segwit and bech32 (even when the scripts are valid)
135+            # We take this as a "good thing" to tell users to upgrade to descriptors.
136+            assert_raises_rpc_error(-4, "Unsupported multisig script size for legacy wallet. Upgrade to descriptors to overcome this limitation for p2sh-segwit or bech32 scripts", wallet_multi.addmultisigaddress, 16, pubkeys, '', 'p2sh-segwit')
137+            assert_raises_rpc_error(-4, "Unsupported multisig script size for legacy wallet. Upgrade to descriptors to overcome this limitation for p2sh-segwit or bech32 scripts", wallet_multi.addmultisigaddress, 16, pubkeys, '', 'bech32')

rkrux commented at 11:14 am on March 31, 2024:

0for addr_type in ['p2sh-segwit', 'bech32']:
1 assert_raises_rpc_error(-4, "Unsupported multisig script size for legacy wallet. Upgrade to descriptors to overcome this limitation for p2sh-segwit or bech32 scripts", wallet_multi.addmultisigaddress, 16, pubkeys, '', addr_type)

in test/functional/rpc_createmultisig.py:113 in a0ebb929e8 outdated

134+            # Due an internal limitation on legacy wallets, the redeem script limit also applies to p2sh-segwit and bech32 (even when the scripts are valid)
135+            # We take this as a "good thing" to tell users to upgrade to descriptors.
136+            assert_raises_rpc_error(-4, "Unsupported multisig script size for legacy wallet. Upgrade to descriptors to overcome this limitation for p2sh-segwit or bech32 scripts", wallet_multi.addmultisigaddress, 16, pubkeys, '', 'p2sh-segwit')
137+            assert_raises_rpc_error(-4, "Unsupported multisig script size for legacy wallet. Upgrade to descriptors to overcome this limitation for p2sh-segwit or bech32 scripts", wallet_multi.addmultisigaddress, 16, pubkeys, '', 'bech32')
138+
139+    def do_multisig(self, nkeys, nsigs, output_type, wallet_multi):

rkrux commented at 11:22 am on March 31, 2024:

Although it’s older code, but would it be possible to rename this function and/or break this function down into smaller pieces since this PR is anyway refactoring this test? I see it’s an 85 line function wherein a lot many things are happening including building/sending transactions, mining blocks, and numerous assertions that are hard to keep track of.

furszy commented at 2:56 pm on April 1, 2024:

Although it’s older code, but would it be possible to rename this function and/or break this function down into smaller pieces since this PR is anyway refactoring this test? I see it’s an 85 line function wherein a lot many things are happening including building/sending transactions, mining blocks, and numerous assertions that are hard to keep track of.

The goal of the function is to test the complete multisig workflow. You will probably end up duplicating code if you decouple it in smaller functions. But feel free to try it locally and push a follow-up PR if you get a better structure.

in test/functional/rpc_createmultisig.py:159 in a0ebb929e8 outdated

199+        # send coins to node2 when wallet is enabled
200+        node2_balance = node2.getbalances()['mine']['trusted'] if self.is_wallet_compiled() else 0
201+        out_addr = node2.getnewaddress() if self.is_wallet_compiled() else getnewdestination('bech32')[2]
202+        rawtx = node2.createrawtransaction([{"txid": tx["txid"], "vout": tx["sent_vout"]}], [{out_addr: outval}])
203 
204         prevtx_err = dict(prevtxs[0])

rkrux commented at 11:24 am on March 31, 2024:

Again older code, but prevtx_err leads to an impression that it’s an error related to a previous transaction but the usage of this variable doesn’t show that it’s an error, but a transaction itself.

furszy commented at 2:57 pm on April 1, 2024:

Again older code, but prevtx_err leads to an impression that it’s an error related to a previous transaction but the usage of this variable doesn’t show that it’s an error, but a transaction itself.

Sure. Will leave it as is to not continue expanding the diff. Feel free to tackle it on a follow-up.

rkrux approved

rkrux commented at 11:40 am on March 31, 2024: none

Ack a0ebb92

Testing Methodology:

Make, successful
All functional tests, successful
Executed createmultisig command with the following output - don’t see Unable to make chosen address type, please ensure no uncompressed public keys are present. warning anymore, nor do I see ERROR: FillableSigningProvider::AddCScript(): redeemScripts > 520 bytes are invalid message in the logs.

0bcli createmultisig 16 "[\"02f293c4d026a536a082453f8327f305ae0a0c59350b396205e15fbec2af67390b\",\"03d7b6a7e972f6e1a532f9f49a7cc35055a0ffd7cec5f81728f6a3fdf4dad93672\",\"021f360d51b8eb43422fe942c837ad16e36bab87c5f7567609de9a38e205fb51ed\",\"03a778eaee9cc18a4e5e02a78f403082d4e6f3a9c0d9edaea20b9ad86089de7fc9\",\"02a3c95e41e8272c6842df8c9da492ef5e03bbc4ca17c22ce1f88570e716e187db\",\"032e1d465bc5cdea674337934fb861d097dd7ab20289e4d41de14a6cafcd61d30a\",\"0370fd35da88778a805ce9acc88c0020e3b14cacaa30f99d829207e1896d782ea1\",\"03878a4b831af4fee069964448ccca999071633fcc751d296e8c5325167274febf\",\"0248f8621596029568070dda40bc1e3ace78df320b5ebf0d6a431536e9af3ec894\",\"03f757260835b7e439b244a9b10f5645966d7dcff9d2648c400a0d6827f4a64ac4\",\"023b3972554e123985b6c3d8575e6fd3384f59a58ef91e0d39ba5905dad49e7372\",\"02e32652928f21764e02822d39ffd1075c428e372a06fe7247023d5b60f1d01596\",\"020e002818c48b1d1766f6196e09db90f51e155bbd49be0ad743d1fa6b7e8efcae\",\"03725d482f0c77f4feb1ecbea28f036b30889f462222002e88aa8f8346dce94474\",\"031fa2798d9f40bc2746f91f300460be9091e10f1b4baa37843cd41c965b0f9b41\",\"0235b7b545398ce9692f319e6eefe114eae50f008f46d33baf19418e0f17e3bc65\",\"02ff72a2b54370c0b1501228b6a210bc36a16535cc358bf7839c85a6ae2c41725c\",\"0282ce44556f5a81c7a698dc8fdba12089740df420af93002e80a752020e5d939e\",\"0279f5b244862e27f69284befb0299909139b899d2702b3e74b4317ed5b2947e03\",\"0293d0b090906afdee07dabedf34798293246ce06ade2e3887302f289008f25f4d\"]" "p2sh-segwit"
1{
2  "address": "2N9mdDwZmrKZNBkX61qidLnPZqkVW8gx3KR",
3  "redeemScript": "602102f293c4d026a536a082453f8327f305ae0a0c59350b396205e15fbec2af67390b2103d7b6a7e972f6e1a532f9f49a7cc35055a0ffd7cec5f81728f6a3fdf4dad9367221021f360d51b8eb43422fe942c837ad16e36bab87c5f7567609de9a38e205fb51ed2103a778eaee9cc18a4e5e02a78f403082d4e6f3a9c0d9edaea20b9ad86089de7fc92102a3c95e41e8272c6842df8c9da492ef5e03bbc4ca17c22ce1f88570e716e187db21032e1d465bc5cdea674337934fb861d097dd7ab20289e4d41de14a6cafcd61d30a210370fd35da88778a805ce9acc88c0020e3b14cacaa30f99d829207e1896d782ea12103878a4b831af4fee069964448ccca999071633fcc751d296e8c5325167274febf210248f8621596029568070dda40bc1e3ace78df320b5ebf0d6a431536e9af3ec8942103f757260835b7e439b244a9b10f5645966d7dcff9d2648c400a0d6827f4a64ac421023b3972554e123985b6c3d8575e6fd3384f59a58ef91e0d39ba5905dad49e73722102e32652928f21764e02822d39ffd1075c428e372a06fe7247023d5b60f1d0159621020e002818c48b1d1766f6196e09db90f51e155bbd49be0ad743d1fa6b7e8efcae2103725d482f0c77f4feb1ecbea28f036b30889f462222002e88aa8f8346dce9447421031fa2798d9f40bc2746f91f300460be9091e10f1b4baa37843cd41c965b0f9b41210235b7b545398ce9692f319e6eefe114eae50f008f46d33baf19418e0f17e3bc652102ff72a2b54370c0b1501228b6a210bc36a16535cc358bf7839c85a6ae2c41725c210282ce44556f5a81c7a698dc8fdba12089740df420af93002e80a752020e5d939e210279f5b244862e27f69284befb0299909139b899d2702b3e74b4317ed5b2947e03210293d0b090906afdee07dabedf34798293246ce06ade2e3887302f289008f25f4d0114ae",
4  "descriptor": "sh(wsh(multi(16,02f293c4d026a536a082453f8327f305ae0a0c59350b396205e15fbec2af67390b,03d7b6a7e972f6e1a532f9f49a7cc35055a0ffd7cec5f81728f6a3fdf4dad93672,021f360d51b8eb43422fe942c837ad16e36bab87c5f7567609de9a38e205fb51ed,03a778eaee9cc18a4e5e02a78f403082d4e6f3a9c0d9edaea20b9ad86089de7fc9,02a3c95e41e8272c6842df8c9da492ef5e03bbc4ca17c22ce1f88570e716e187db,032e1d465bc5cdea674337934fb861d097dd7ab20289e4d41de14a6cafcd61d30a,0370fd35da88778a805ce9acc88c0020e3b14cacaa30f99d829207e1896d782ea1,03878a4b831af4fee069964448ccca999071633fcc751d296e8c5325167274febf,0248f8621596029568070dda40bc1e3ace78df320b5ebf0d6a431536e9af3ec894,03f757260835b7e439b244a9b10f5645966d7dcff9d2648c400a0d6827f4a64ac4,023b3972554e123985b6c3d8575e6fd3384f59a58ef91e0d39ba5905dad49e7372,02e32652928f21764e02822d39ffd1075c428e372a06fe7247023d5b60f1d01596,020e002818c48b1d1766f6196e09db90f51e155bbd49be0ad743d1fa6b7e8efcae,03725d482f0c77f4feb1ecbea28f036b30889f462222002e88aa8f8346dce94474,031fa2798d9f40bc2746f91f300460be9091e10f1b4baa37843cd41c965b0f9b41,0235b7b545398ce9692f319e6eefe114eae50f008f46d33baf19418e0f17e3bc65,02ff72a2b54370c0b1501228b6a210bc36a16535cc358bf7839c85a6ae2c41725c,0282ce44556f5a81c7a698dc8fdba12089740df420af93002e80a752020e5d939e,0279f5b244862e27f69284befb0299909139b899d2702b3e74b4317ed5b2947e03,0293d0b090906afdee07dabedf34798293246ce06ade2e3887302f289008f25f4d)))#xfjxuvgn"
5}

pinheadmz approved

pinheadmz commented at 3:37 pm on May 3, 2024: member

ACK a0ebb929e865903ca1cc2674e74906a806e73109

Diff since last ACK is minimal, just nits in tests. Since it’s been a while, I re-reviewed the whole PR anyway. Most of the changes are cleanup refactors in the relevant tests which I agree are worthwhile. The main bugfix is switching from FillableSigningProvider to FlatSigningProvider when adding a CScript inAddAndGetDestinationForScript(), because the latter does not require that redeemScript.size() <= MAX_SCRIPT_ELEMENT_SIZE

The script element size check is preserved (appropriately only for LEGACY output types) in the calling function AddAndGetMultisigDestination()

Confirmed the test fails on master with redeemScripts > 520 bytes are invalid (had to manually revert a few refactors to run it on master) and passes on the branch.

Also ran the createmultisig RPC on PR branch and master to ensure the bug is fixed: https://gist.github.com/pinheadmz/856f94c1c483343f63e6f144638ac96f

I do have one concern which is that RPC signrawtransactionwithkey is modified but I do not think it is tested directly with a legacy multisig with > 15 keys. Such a template would have to be constructed manually because our own RPC createmultisig will not do it. Just looking at the code I think it would be possible for a user to sign an invalid legacy transaction, if they could find a way to construct one. I may try to write this test later today and will report back!

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA256
 2
 3ACK a0ebb929e865903ca1cc2674e74906a806e73109
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmY0/dIACgkQ5+KYS2KJ
 7yTrKJg/+PuYRj9moqfODqMb2nXQwhsS6qMp8iLA77aNLiXGTZzOl1b3T3JqP0/6l
 81sgdfrqHNMnqJ9HCkazLUcx5a5ZyK/3QjmxhgrxybmQ+m1QFr+GfHqZjehekgWP+
 9kgpJ9YSFL3i/lNCQZBNCnZfFynK6zb+jD/5NSPWEWIDM/C8VMGWNr6ZvOyOJZvH7
10YvV4gJx3CGYT8n0KGIyDDU6PdxHqspa6pHjhmVgN0Uy/ghaiofC7IuNSznJ7R6xH
11PLPRArXh9u3kO8gzPU1NJjnzrclQZnfHHbIqP5gk5uXBFXU2IcMaWt0hfYeWtOr+
12Jw0nVQj5yd2RH+TpsJbrEy+hfhjQL0ZtoXcZp21/psD/GhEP/ke0kcVDza6t+WJX
13qmvw6HZ0Shxi0wiGqtRTpKTfDxA5PXBcdARQljVE0eIgw+dg8neKW1vYFLZQ1bdG
149f9qsusf1QoMDzP2fomYYBYgtpCdb6q+IBigNVJ0+pmEzvrhs6L8BKau5Yvf7AEo
1579BkSmT11l+1X+E+xl2rxB8j4GIMItto6DG57sGt5VlTzkxqmxvY7fPEpwsYBEOQ
16uOjVcIgdyNsdvR5h5rKfu6KMCTtLurC/ny56H3dqhbAH/uuRYAESP/g1JNZ+2n2n
17pk8cmwdSpRHFnlm0B5YWDOHhWFK+HAq/wTbsPvho2rWhsY88cYU=
18=Aora
19-----END PGP SIGNATURE-----

pinheadmz’s public key is on keybase

DrahtBot requested review from theStack on May 3, 2024

DrahtBot added the label Needs rebase on May 3, 2024

test: rpc_createmultisig, remove manual wallet initialization

There is no need to manually initialize the wallets within the test
case. The test framework already initializes them when `_requires_wallet`
is true.

3635d43268

test: refactor, multiple cleanups in rpc_createmultisig.py

Cleaning up the test in the following ways:

* Generate priv-pub key pairs used for testing only once (instead of doing it 4 times).
* Simplifies 'wmulti' wallet creation, load and unload process.
* Removes confusing class members initialized and updated inside a nested for-loop.
* Simplifies do_multisig() outpoint detection:
  The outpoint index information is already contained in MiniWallet's
  `send_to` return value dictionary as "sent_vout".

Co-authored-by: Sebastian Falbesoner <sebastian.falbesoner@gmail.com>

b5a3289433

test: rpc_createmultisig, remove unnecessary checkbalances()

The function exists merely to check that the node2's wallet
received the transactions created during all the 'do_multisig()'
calls.
It was created as a standalone function because 'getbalance()'
only returns something when transactions are confirmed. So,
the rationale on that time was to have a method mining blocks
to confirm the recently created transactions to be able to
check the incoming balance.
This is why we have the "moved" class field.

This change removes all the hardcoded amounts and verifies
node2 balance reception directly inside 'do_multisig()'.

25a81705d3

test: rpc_createmultisig, decouple 'test_mixing_uncompressed_and_compressed_keys'

And also, simplified the test a bit by re-using the already existing 'wallet_multi'
(instead of creating a new one). Plus, removed the 'is_bdb_compiled()' calls
which were there basically to check if the test has the wallet compiled or not.

4f33dbd8f8

test: rpc_createmultisig, decouple 'test_sortedmulti_descriptors_bip67'

Move-only commit. No behavior change.

f7a173b578

fix incorrect multisig redeem script size limit for segwit

The multisig script generation process currently fails when
the user exceeds 15 keys, even when it shouldn't. The maximum
number of keys allowed for segwit redeem scripts (p2sh-segwit
and bech32) is 20 keys.
This is because the redeem script placed in the witness is not
restricted by the item size limit.

The reason behind this issue is the utilization of the legacy
p2sh redeem script restrictions on segwit ones. Redeem scripts
longer than 520 bytes are blocked from being inserted into the
keystore, which causes the signing process and the descriptor
inference process to fail.

This occurs because the multisig generation flow uses the same
keystore as the legacy spkm (FillableSigningProvider), which
contains the 520-byte limit.

0c9fedfc45

rpc: bugfix, incorrect segwit redeem script size used in signrawtransactionwithkey

The process currently fails to sign redeem scripts that are longer than
520 bytes. Even when it shouldn't. The 520 bytes redeem scripts limit
is a legacy p2sh rule, and not a segwit limitation.

Segwit redeem scripts are not restricted by the script item size limit.

The reason why this occurs, is the usage of the same keystore used by
the legacy spkm. Which contains blockage for any redeem scripts longer
than the script item size limit.

9d9a91c4ea

test: coverage for 16-20 segwit multisig scripts

This exercises the bug fixed by previous commits, where
we were unable to generate and sign for segwit redeem scripts
(in this case multisig redeem scripts) longer than 520 bytes.

and also, this adds coverage for legacy 15-15 multisig script
generation and signing.

9be6065cc0

bugfix: addmultisigaddress, add unsupported operation for redeem scripts over 520 bytes

Due to a bug in the legacy wallet, the p2sh maximum script size limit is also imposed
on 'p2sh-segwit' and 'bech32' redeem scripts.
Although redeem scripts over MAX_SCRIPT_ELEMENT_SIZE bytes are technically valid for
segwit output types, we don't want to enable this feature in legacy wallets for the
following reasons:

1) It introduces a compatibility-breaking change requiring downgrade protection; older
   wallets would be unable to interact with these "new" legacy wallets.

2) Considering the ongoing deprecation of the legacy spkm, this issue adds another
   good reason to transition towards descriptors.

53302a0981

test: addmultisigaddress, coverage for script size limits 2451a217dd

furszy force-pushed on May 3, 2024

furszy commented at 5:25 pm on May 3, 2024: member

sad rebase after #30024 merge.

pinheadmz commented at 5:36 pm on May 3, 2024: member

I wrote a test for RPC signrawtransactionwithkey that covers legacy P2SH with 15 and 16 public keys. Both calls succeed even though the latter produces a scriptSig that exceeds MAX_STANDARD_SCRIPTSIG_SIZE and if it ever made it into a block, would exceed MAX_SCRIPT_ELEMENT_SIZE

This test behaves the same on master! So, that is potentially a footgun although not an easy one for a user to pull off and also would not result in any loss of funds.

https://gist.github.com/pinheadmz/ca8ed75913902bd1ddf88bdd921712ee

DrahtBot removed the label Needs rebase on May 3, 2024

furszy commented at 2:57 pm on May 5, 2024: member

I wrote a test for RPC signrawtransactionwithkey that covers legacy P2SH with 15 and 16 public keys. Both calls succeed even though the latter produces a scriptSig that exceeds MAX_STANDARD_SCRIPTSIG_SIZE and if it ever made it into a block, would exceed MAX_SCRIPT_ELEMENT_SIZE

The second call, the one with 16 pubkeys, doesn’t entirely succeed. The test isn’t checking the error field in the signrawtransactionwithkey response, which returns an 'error': 'Push value size limit exceeded'.

The confusing point about this command is that it returns the post-processed tx hex regardless of what it was able to do internally or if the script passes the verification step. This is because, in a multisig scenario, the process could have appended only one of the signatures, which would obviously fail during the script verification. Thus, the process needs to update the transaction and notify the user that there are still missing signatures so it can relay it to the next party.

I think we could improve the signing process so it does not update the returned transaction when the verification step finds an error that will persist regardless of any following-up processing step.

That being said, I would encourage people to use PSBT instead of signrawtransactionwithkey.

pinheadmz commented at 3:31 pm on May 6, 2024: member

The confusing point about this command is that it returns the post-processed tx hex regardless of what it was able to do internally or if the script passes the verification step. This is because, in a multisig scenario, the process could have appended only one of the signatures, which would obviously fail during the script verification. Thus, the process needs to update the transaction and notify the user that there are still missing signatures so it can relay it to the next party.

Ah thanks, TIL! Now I see how this PR affects that specific RPC

master

Does not sign, tx remains invalid *

{‘hex’: ‘0200000001da5a0d84190834f2e417ac124a7fb0cf386f73fdc3f0dec5320e332b1acc2df90000000000fdffffff01c0e4022a01000000225120d8f28c020ca2c2d17f27eff0ccb1131382e061d5f6f4c2bad4608f96ca7c68a700000000’, ‘complete’: False, ’errors’: [{’txid’: ‘f92dcc1a2b330e32c5def0c3fd736f38cfb07f4a12ac17e4f2340819840d5ada’, ‘vout’: 0, ‘witness’: [], ‘scriptSig’: ‘’, ‘sequence’: 4294967293, ’error’: ‘Unable to sign input, invalid stack size (possibly missing key)’}]}

PR 28307

Signs, creating tx that is invalid for a different reason *

{‘hex’: ‘0200000001748d3c4f36e30d8d490e87c7f736e2fd818f18505f2e7b670d24aa8d52b61d7800000000fd6f020047304402205573e3358c93b899f5c91ed048363e388bee1d2dc79b65db9a907b71736a2ac9022021878bbee92ef3cd86d1d13a622948f8295b4de3ee0d27ddce67266351cd0e93014d23025121022ad2cbe6eb900100e0687f3d5c24d5af05a3f51cb304653833852d4b786219c42102b907c238e932585f89620f9d22e43e3c79814e20b1f19943bb16fc9ec9fa5b7b2102133d3548c34804b57127308980070929b29c2c273a995c7b19370776528c482f21034a218ed831df40b0d51acde6f60f3eb4fb1fd774528df5d8225d64c817e2eafc2103d91c55070d379c358e7a5ec216c75d512b864872f0722c5cc41e261de60ab36f2103f687e273c93aad1cb3c0e6837a95cb3532b2745e7315342c8a80e2683cbf07852102a5a4a70e4ef942a69c3e2e246256f41da149a7ab2f69a2b5c8fba09247cbea34210354a67b554c251e238faf2ec91532ae0e9783f89451cd5ac8df483da8cf4ba42f21022b702b20654cd110f77f7dbbe4f89a5629baca8f83c4bd3580332534381036b321028ad603a203645db07e76dc2ff73a0c1a628d215df67cc9f57923e5bfedc09a33210289de87715d75d12c931791e91091e33a292dbac7be19f7998e62eafe961b0b87210339175fa8aeee2d525e6414bff501a09b8ab2fac58948f3cf99bd285489467804210355cdc7bb582a8434d8c2b71e3ff8a6029cf26ae54f5058cd96025f1ac12bba9721026c4da02cfe442ea4e58c48bdb8867612ba62a06821f018c2357d4424c5e5ceaa21039db5509208500013366ad60684f7e4f63a768b48b6b06cb83ce549383c6307542103049efb80896116bd2ee7de7255e018fef52cf982d5fbd60f4b1fb777ed9efa3a60aefdffffff01c0e4022a01000000225120d61857a69972ddac6af7e01798f2eea20a66c8368f51871016f1f706778bf4fc00000000’, ‘complete’: False, ’errors’: [{’txid’: ‘781db6528daa240d677b2e5f50188f81fde236f7c7870e498d0de3364f3c8d74’, ‘vout’: 0, ‘witness’: [], ‘scriptSig’: ‘0047304402205573e3358c93b899f5c91ed048363e388bee1d2dc79b65db9a907b71736a2ac9022021878bbee92ef3cd86d1d13a622948f8295b4de3ee0d27ddce67266351cd0e93014d23025121022ad2cbe6eb900100e0687f3d5c24d5af05a3f51cb304653833852d4b786219c42102b907c238e932585f89620f9d22e43e3c79814e20b1f19943bb16fc9ec9fa5b7b2102133d3548c34804b57127308980070929b29c2c273a995c7b19370776528c482f21034a218ed831df40b0d51acde6f60f3eb4fb1fd774528df5d8225d64c817e2eafc2103d91c55070d379c358e7a5ec216c75d512b864872f0722c5cc41e261de60ab36f2103f687e273c93aad1cb3c0e6837a95cb3532b2745e7315342c8a80e2683cbf07852102a5a4a70e4ef942a69c3e2e246256f41da149a7ab2f69a2b5c8fba09247cbea34210354a67b554c251e238faf2ec91532ae0e9783f89451cd5ac8df483da8cf4ba42f21022b702b20654cd110f77f7dbbe4f89a5629baca8f83c4bd3580332534381036b321028ad603a203645db07e76dc2ff73a0c1a628d215df67cc9f57923e5bfedc09a33210289de87715d75d12c931791e91091e33a292dbac7be19f7998e62eafe961b0b87210339175fa8aeee2d525e6414bff501a09b8ab2fac58948f3cf99bd285489467804210355cdc7bb582a8434d8c2b71e3ff8a6029cf26ae54f5058cd96025f1ac12bba9721026c4da02cfe442ea4e58c48bdb8867612ba62a06821f018c2357d4424c5e5ceaa21039db5509208500013366ad60684f7e4f63a768b48b6b06cb83ce549383c6307542103049efb80896116bd2ee7de7255e018fef52cf982d5fbd60f4b1fb777ed9efa3a60ae’, ‘sequence’: 4294967293, ’error’: ‘Push value size limit exceeded’}]}

pinheadmz approved

pinheadmz commented at 3:39 pm on May 6, 2024: member

ACK 2451a217dd2c21b6d2f2b2699ceddd0bf9073019

Minimal diff since last ACK, just rebase on master and replace a"520" with a “MAX_SCRIPT_ELEMENT_SIZE”

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA256
 2
 3ACK 2451a217dd2c21b6d2f2b2699ceddd0bf9073019
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmY4+ZIACgkQ5+KYS2KJ
 7yTqhnhAApaBq0dhQVXvLaTSBjV8Q8CXJTXapgme6uII649DlRjCQujM4ufmnPfdR
 8BuQWzAC5FbgA1sCkRk3+D/CyO0H9k5PPmU7Mle6tQl7sHszkXNqzCXmIceQBPvQk
 9Qk95jzcrh88nJif565D3o88+RGaREku8mGWG5HCkcSyyfTMwaXdxVkvfPB88+g8l
10fTDcsP+P7At/fKOIBEN8UHX4hkKhWAv4WYXVI3qUXBWix6wSNqYrQNGwj9BPSHtQ
116pA9p0kZ15SP8cJo0Fe/Hfb2Zk9vtXbuIUGxHMSu08ka+9C8+OlOqJ6NFB/dnTAh
12GX60UiC6QXPOvIUZrtC4JWHkdNRJ3fNV+E3xQ8eBlPMMT8ZsznY+SG4lrVYb1ZKG
13D0Ho/ASiGYaN4kOTZOVDtuZdspDFBy2z2epmXpBn7h+P1OXgyz1KlGO4gIydrdkO
14ITVORmyR2/c1poUyJhHWMaf2pdEYfX0T+UWNvkL+Do0iIDL5JgJCPULfA2T8/FjI
155TjZAVVH1it+BKBtY4owk0LLSiVJfvBGkbPpJaghUYKIrZHiREj8nMq/5+JTHPai
16g6JP3lUNEDVi9EPStYWScbIAeoeiy/gFp8fGhST0ovPwN44Cj0/LQdjnvUb1hjS8
17O0Y7JGtVBf62UwHItdGTP9rIiuD+oePf5iC7PIBJff1RwjF6ADc=
18=IlyD
19-----END PGP SIGNATURE-----

pinheadmz’s public key is on keybase

in src/wallet/rpc/addresses.cpp:310 in 53302a0981 outdated

306+
307+        if (!spk_man.AddCScript(script)) {
308+            if (CScript inner_script; spk_man.GetCScript(CScriptID(script), inner_script)) {
309+                CHECK_NONFATAL(inner_script == script); // Nothing to add, script already contained by the wallet
310+                continue;
311+            }

theStack commented at 2:37 pm on May 17, 2024:

It took me a while to grok what’s going on here: if the CScript was already contained in the wallet, the AddCScript call one line above would fail in the course of writing to the DB (looking at the call flow LegacyScriptPubKeyMan::AddCScript -> LegacyScriptPubKeyMan::AddCScriptWithDB -> batch.WriteCScript(...) -> WriteIC(..., ..., false), where the last false parameter indicates that overwrites are not allowed; the FillableSigningProvider::AddCScript call earlier doesn’t care if an entry is overwritten and still returns true).

I think the same behaviour could be achieved by simply ignoring the return value of AddCScript, but I assume the intention here is to be more robust and error if – for whatever reason – the CScript write to the database failed, even if the entry isn’t there yet.

theStack approved

theStack commented at 3:53 pm on May 17, 2024: contributor

Code-review ACK 2451a217dd2c21b6d2f2b2699ceddd0bf9073019

in test/functional/rpc_createmultisig.py:181 in b5a3289433 outdated

196-        if self.output_type == 'bech32':
197+        if output_type == 'bech32':
198             assert madd[0:4] == "bcrt"  # actually a bech32 address
199 
200-        if self.is_bdb_compiled():
201+        if wallet_multi is not None:

achow101 commented at 0:46 am on June 5, 2024:

In b5a328943362cfac6e90fd4e1b167c357d53b7d4 “test: refactor, multiple cleanups in rpc_createmultisig.py”

Strictly speaking, this should also check for whether we have legacy wallets since the test is specifically for whether addmultisigaddress (a legacy wallet only rpc) matches createmultisig.

0        if wallet_multi is not None and not self.options.descriptors:

in test/functional/rpc_createmultisig.py:142 in 9be6065cc0 outdated

139         prevtxs = [{"txid": tx["txid"], "vout": tx["sent_vout"], "scriptPubKey": spk.hex(), "redeemScript": mredeem, "amount": value}]
140 
141         self.generate(node0, 1)
142 
143-        outval = value - decimal.Decimal("0.00001000")
144+        outval = value - decimal.Decimal("0.00002000")  # deduce fee (must be higher than the min relay fee)

achow101 commented at 1:02 am on June 5, 2024:

In 9be6065cc03f2408f290a332b203eef9c9cebf24 “test: coverage for 16-20 segwit multisig scripts”

nit: s/deduce/deduct

achow101 commented at 1:12 am on June 5, 2024: member

ACK 2451a217dd2c21b6d2f2b2699ceddd0bf9073019

achow101 merged this on Jun 5, 2024

achow101 closed this on Jun 5, 2024

furszy deleted the branch on Jun 5, 2024

rpc, wallet: fix incorrect segwit redeem script size limit #28307

Code Coverage

Reviews

Conflicts

master

PR 28307