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

furszy · 2023-08-21T23:18:02Z

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 · 2023-08-21T23:18:05Z

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

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.

pinheadmz · 2023-09-06T16:29:01Z

concept ACK db38b58

Confirmed this branch fixes the issue mentioned in #28250

Code review to follow...

{
  "address": "3JDRACdkEs41yxtYLi6kiqQJdQHLPDxehc",
  "redeemScript": "602102f293c4d026a536a082453f8327f305ae0a0c59350b396205e15fbec2af67390b2103d7b6a7e972f6e1a532f9f49a7cc35055a0ffd7cec5f81728f6a3fdf4dad9367221021f360d51b8eb43422fe942c837ad16e36bab87c5f7567609de9a38e205fb51ed2103a778eaee9cc18a4e5e02a78f403082d4e6f3a9c0d9edaea20b9ad86089de7fc92102a3c95e41e8272c6842df8c9da492ef5e03bbc4ca17c22ce1f88570e716e187db21032e1d465bc5cdea674337934fb861d097dd7ab20289e4d41de14a6cafcd61d30a210370fd35da88778a805ce9acc88c0020e3b14cacaa30f99d829207e1896d782ea12103878a4b831af4fee069964448ccca999071633fcc751d296e8c5325167274febf210248f8621596029568070dda40bc1e3ace78df320b5ebf0d6a431536e9af3ec8942103f757260835b7e439b244a9b10f5645966d7dcff9d2648c400a0d6827f4a64ac421023b3972554e123985b6c3d8575e6fd3384f59a58ef91e0d39ba5905dad49e73722102e32652928f21764e02822d39ffd1075c428e372a06fe7247023d5b60f1d0159621020e002818c48b1d1766f6196e09db90f51e155bbd49be0ad743d1fa6b7e8efcae2103725d482f0c77f4feb1ecbea28f036b30889f462222002e88aa8f8346dce9447421031fa2798d9f40bc2746f91f300460be9091e10f1b4baa37843cd41c965b0f9b41210235b7b545398ce9692f319e6eefe114eae50f008f46d33baf19418e0f17e3bc652102ff72a2b54370c0b1501228b6a210bc36a16535cc358bf7839c85a6ae2c41725c210282ce44556f5a81c7a698dc8fdba12089740df420af93002e80a752020e5d939e210279f5b244862e27f69284befb0299909139b899d2702b3e74b4317ed5b2947e03210293d0b090906afdee07dabedf34798293246ce06ade2e3887302f289008f25f4d0114ae",
  "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"
}

src/outputtype.cpp

test/functional/rpc_createmultisig.py

furszy

Updated per feedback. Thanks @pinheadmz. Small diff .

Added coverage for createmultisig +20 keys error.

theStack

Concept ACK

test/functional/test_framework/test_framework.py

furszy

Updated per feedback. Thanks theStack.

rkrux

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.

bcli createmultisig 16 "[\"02f293c4d026a536a082453f8327f305ae0a0c59350b396205e15fbec2af67390b\",\"03d7b6a7e972f6e1a532f9f49a7cc35055a0ffd7cec5f81728f6a3fdf4dad93672\",\"021f360d51b8eb43422fe942c837ad16e36bab87c5f7567609de9a38e205fb51ed\",\"03a778eaee9cc18a4e5e02a78f403082d4e6f3a9c0d9edaea20b9ad86089de7fc9\",\"02a3c95e41e8272c6842df8c9da492ef5e03bbc4ca17c22ce1f88570e716e187db\",\"032e1d465bc5cdea674337934fb861d097dd7ab20289e4d41de14a6cafcd61d30a\",\"0370fd35da88778a805ce9acc88c0020e3b14cacaa30f99d829207e1896d782ea1\",\"03878a4b831af4fee069964448ccca999071633fcc751d296e8c5325167274febf\",\"0248f8621596029568070dda40bc1e3ace78df320b5ebf0d6a431536e9af3ec894\",\"03f757260835b7e439b244a9b10f5645966d7dcff9d2648c400a0d6827f4a64ac4\",\"023b3972554e123985b6c3d8575e6fd3384f59a58ef91e0d39ba5905dad49e7372\",\"02e32652928f21764e02822d39ffd1075c428e372a06fe7247023d5b60f1d01596\",\"020e002818c48b1d1766f6196e09db90f51e155bbd49be0ad743d1fa6b7e8efcae\",\"03725d482f0c77f4feb1ecbea28f036b30889f462222002e88aa8f8346dce94474\",\"031fa2798d9f40bc2746f91f300460be9091e10f1b4baa37843cd41c965b0f9b41\",\"0235b7b545398ce9692f319e6eefe114eae50f008f46d33baf19418e0f17e3bc65\",\"02ff72a2b54370c0b1501228b6a210bc36a16535cc358bf7839c85a6ae2c41725c\",\"0282ce44556f5a81c7a698dc8fdba12089740df420af93002e80a752020e5d939e\",\"0279f5b244862e27f69284befb0299909139b899d2702b3e74b4317ed5b2947e03\",\"0293d0b090906afdee07dabedf34798293246ce06ade2e3887302f289008f25f4d\"]" "p2sh-segwit"
{
  "address": "2N9mdDwZmrKZNBkX61qidLnPZqkVW8gx3KR",
  "redeemScript": "602102f293c4d026a536a082453f8327f305ae0a0c59350b396205e15fbec2af67390b2103d7b6a7e972f6e1a532f9f49a7cc35055a0ffd7cec5f81728f6a3fdf4dad9367221021f360d51b8eb43422fe942c837ad16e36bab87c5f7567609de9a38e205fb51ed2103a778eaee9cc18a4e5e02a78f403082d4e6f3a9c0d9edaea20b9ad86089de7fc92102a3c95e41e8272c6842df8c9da492ef5e03bbc4ca17c22ce1f88570e716e187db21032e1d465bc5cdea674337934fb861d097dd7ab20289e4d41de14a6cafcd61d30a210370fd35da88778a805ce9acc88c0020e3b14cacaa30f99d829207e1896d782ea12103878a4b831af4fee069964448ccca999071633fcc751d296e8c5325167274febf210248f8621596029568070dda40bc1e3ace78df320b5ebf0d6a431536e9af3ec8942103f757260835b7e439b244a9b10f5645966d7dcff9d2648c400a0d6827f4a64ac421023b3972554e123985b6c3d8575e6fd3384f59a58ef91e0d39ba5905dad49e73722102e32652928f21764e02822d39ffd1075c428e372a06fe7247023d5b60f1d0159621020e002818c48b1d1766f6196e09db90f51e155bbd49be0ad743d1fa6b7e8efcae2103725d482f0c77f4feb1ecbea28f036b30889f462222002e88aa8f8346dce9447421031fa2798d9f40bc2746f91f300460be9091e10f1b4baa37843cd41c965b0f9b41210235b7b545398ce9692f319e6eefe114eae50f008f46d33baf19418e0f17e3bc652102ff72a2b54370c0b1501228b6a210bc36a16535cc358bf7839c85a6ae2c41725c210282ce44556f5a81c7a698dc8fdba12089740df420af93002e80a752020e5d939e210279f5b244862e27f69284befb0299909139b899d2702b3e74b4317ed5b2947e03210293d0b090906afdee07dabedf34798293246ce06ade2e3887302f289008f25f4d0114ae",
  "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"
}

src/wallet/rpc/addresses.cpp

test/functional/rpc_createmultisig.py

pinheadmz

ACK a0ebb92

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!

Show Signature

pinheadmz's public key is on keybase

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

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>

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()'.

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

Move-only commit. No behavior change.

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.

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

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.

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

furszy

sad rebase after #30024 merge.

pinheadmz · 2024-05-03T17:36:01Z

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

furszy

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 · 2024-05-06T15:31:26Z

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

ACK 2451a21

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

Show Signature

pinheadmz's public key is on keybase

theStack

Code-review ACK 2451a21

theStack · 2024-05-17T14:37:17Z

src/wallet/rpc/addresses.cpp

+            if (CScript inner_script; spk_man.GetCScript(CScriptID(script), inner_script)) {
+                CHECK_NONFATAL(inner_script == script); // Nothing to add, script already contained by the wallet
+                continue;
+            }


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.

test/functional/rpc_createmultisig.py

DrahtBot added the CI failed label Aug 22, 2023

furszy force-pushed the 2023_invalid_segwit_redeem_script_limit branch from 76a83e7 to db38b58 Compare August 22, 2023 01:04

theStack mentioned this pull request Aug 22, 2023

test: fix keys_to_multisig_script (P2MS) helper for n/k > 16 #28312

Open

DrahtBot removed the CI failed label Aug 22, 2023

furszy mentioned this pull request Aug 22, 2023

RPC createmultisig outputs a sh(addr(...)) descriptor when address_type field is "p2sh-segwit" #28250

Closed

1 task

DrahtBot mentioned this pull request Aug 22, 2023

Support JSON-RPC 2.0 when requested by client #27101

Merged

glozow added the Wallet label Sep 1, 2023

DrahtBot mentioned this pull request Sep 2, 2023

test: Use pathlib over os path #28392

Merged

DrahtBot added CI failed and removed CI failed labels Sep 3, 2023

pinheadmz reviewed Sep 6, 2023

View reviewed changes

src/outputtype.cpp Show resolved Hide resolved

test/functional/rpc_createmultisig.py Show resolved Hide resolved

furszy force-pushed the 2023_invalid_segwit_redeem_script_limit branch 2 times, most recently from cc056d3 to 4c42ab5 Compare September 6, 2023 21:00

furszy commented Sep 6, 2023

View reviewed changes

DrahtBot mentioned this pull request Sep 25, 2023

test: Use test framework utils in functional tests #28528

Closed

DrahtBot mentioned this pull request Oct 24, 2023

Remove the legacy wallet and BDB dependency #28710

Draft

DrahtBot added the CI failed label Jan 12, 2024

DrahtBot mentioned this pull request Jan 18, 2024

Wallet: Add maxfeerate wallet startup option #29278

Open

furszy mentioned this pull request Jan 22, 2024

script/sign: avoid duplicated signature verification after signing (+introduce signing benchmarks) #28923

Open

theStack reviewed Jan 26, 2024

View reviewed changes

test/functional/test_framework/test_framework.py Outdated Show resolved Hide resolved

furszy force-pushed the 2023_invalid_segwit_redeem_script_limit branch from 4c42ab5 to a0ebb92 Compare January 26, 2024 13:56

furszy commented Jan 26, 2024

View reviewed changes

DrahtBot removed the CI failed label Jan 26, 2024

rkrux approved these changes Mar 31, 2024

View reviewed changes

This was referenced May 2, 2024

test: use assert_greater_than util #30019

Open

doc: replace remaining "520" magic nums with MAX_SCRIPT_ELEMENT_SIZE #30024

Merged

pinheadmz approved these changes May 3, 2024

View reviewed changes

DrahtBot requested a review from theStack May 3, 2024 15:37

DrahtBot added the Needs rebase label May 3, 2024

furszy and others added 10 commits May 3, 2024 14:19

test: rpc_createmultisig, remove manual wallet initialization

3635d43

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

test: rpc_createmultisig, decouple 'test_sortedmulti_descriptors_bip67'

f7a173b

Move-only commit. No behavior change.

test: addmultisigaddress, coverage for script size limits

2451a21

furszy force-pushed the 2023_invalid_segwit_redeem_script_limit branch from a0ebb92 to 2451a21 Compare May 3, 2024 17:24

furszy commented May 3, 2024

View reviewed changes

DrahtBot removed the Needs rebase label May 3, 2024

furszy commented May 5, 2024

View reviewed changes

pinheadmz approved these changes May 6, 2024

View reviewed changes

theStack approved these changes May 17, 2024

View reviewed changes

furszy mentioned this pull request May 25, 2024

Enable importprivkey, addmultisigaddress in descriptor wallets #30175

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

furszy commented Aug 21, 2023 •

edited

DrahtBot commented Aug 21, 2023 •

edited

pinheadmz commented Sep 6, 2023

furszy left a comment

theStack left a comment

furszy left a comment

rkrux left a comment

pinheadmz left a comment

furszy left a comment

pinheadmz commented May 3, 2024

furszy left a comment

pinheadmz commented May 6, 2024

pinheadmz left a comment

theStack left a comment

theStack May 17, 2024

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

Are you sure you want to change the base?

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

Conversation

furszy commented Aug 21, 2023 • edited

DrahtBot commented Aug 21, 2023 • edited

Code Coverage

Reviews

Conflicts

pinheadmz commented Sep 6, 2023

furszy left a comment

Choose a reason for hiding this comment

theStack left a comment

Choose a reason for hiding this comment

furszy left a comment

Choose a reason for hiding this comment

rkrux left a comment

Choose a reason for hiding this comment

pinheadmz left a comment

Choose a reason for hiding this comment

furszy left a comment

Choose a reason for hiding this comment

pinheadmz commented May 3, 2024

furszy left a comment

Choose a reason for hiding this comment

pinheadmz commented May 6, 2024

master

PR 28307

pinheadmz left a comment

Choose a reason for hiding this comment

theStack left a comment

Choose a reason for hiding this comment

theStack May 17, 2024

Choose a reason for hiding this comment

furszy commented Aug 21, 2023 •

edited

DrahtBot commented Aug 21, 2023 •

edited