test: wallet: cover wallet passphrase with a null char #32675

brunoerg commented at 5:17 PM on June 3, 2025: contributor

This PR adds test coverage for the walletpassphrase/walletpassphrasechange RPC when the passphrase is incorrect due to a null character.

For reference: #27068 introduced the usage of SecureString to allow null characters.

DrahtBot commented at 5:17 PM on June 3, 2025: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32675.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	maflcko, w0xlt, BrandonOdiwuor, theStack, pablomartin4btc, 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:

#32290 (test: allow all functional tests to be run or skipped with --usecli by mzumsande)

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 Tests on Jun 3, 2025

test: wallet: cover wallet passphrase with a null char 7cfbb8575e

in test/functional/wallet_encryption.py:99 in 3b90060a2d outdated

  94 | @@ -95,6 +95,7 @@ def run_test(self):
  95 |          self.nodes[0].walletpassphrasechange(passphrase2, passphrase_with_nulls)
  96 |          # walletpassphrasechange should not stop at null characters
  97 |          assert_raises_rpc_error(-14, "wallet passphrase entered was incorrect", self.nodes[0].walletpassphrase, passphrase_with_nulls.partition("\0")[0], 10)
  98 | +        assert_raises_rpc_error(-14, "wallet passphrase entered is incorrect. It contains a null character (ie - a zero byte)", self.nodes[0].walletpassphrase, passphrase_with_nulls + "\0", 10)

maflcko commented at 7:39 PM on June 3, 2025:

might as well add coverage for walletpassphrasechange as well?

brunoerg commented at 12:36 PM on June 4, 2025:

Yes, going to add it.

brunoerg commented at 1:07 PM on June 4, 2025:

Done

brunoerg force-pushed on Jun 4, 2025

brunoerg commented at 1:08 PM on June 4, 2025: contributor

3b90060a2df2b65369117f3ee8afe26be631867d..7cfbb8575e1ffbad5c48e2c461b45dd6ac63d064 address #32675 (review)

maflcko commented at 1:11 PM on June 4, 2025: member

lgtm ACK 7cfbb8575e1ffbad5c48e2c461b45dd6ac63d064

w0xlt commented at 11:39 PM on June 4, 2025: contributor

Code review ACK https://github.com/bitcoin/bitcoin/pull/32675/commits/7cfbb8575e1ffbad5c48e2c461b45dd6ac63d064

BrandonOdiwuor commented at 10:37 AM on June 5, 2025: contributor

Code Review ACK 7cfbb8575e1ffbad5c48e2c461b45dd6ac63d064

in test/functional/wallet_encryption.py:100 in 7cfbb8575e

  94 | @@ -95,6 +95,9 @@ def run_test(self):
  95 |          self.nodes[0].walletpassphrasechange(passphrase2, passphrase_with_nulls)
  96 |          # walletpassphrasechange should not stop at null characters
  97 |          assert_raises_rpc_error(-14, "wallet passphrase entered was incorrect", self.nodes[0].walletpassphrase, passphrase_with_nulls.partition("\0")[0], 10)
  98 | +        assert_raises_rpc_error(-14, "The wallet passphrase entered was incorrect", self.nodes[0].walletpassphrasechange, passphrase_with_nulls.partition("\0")[0], "abc")
  99 | +        assert_raises_rpc_error(-14, "wallet passphrase entered is incorrect. It contains a null character (ie - a zero byte)", self.nodes[0].walletpassphrase, passphrase_with_nulls + "\0", 10)
 100 | +        assert_raises_rpc_error(-14, "The old wallet passphrase entered is incorrect. It contains a null character (ie - a zero byte)", self.nodes[0].walletpassphrasechange, passphrase_with_nulls + "\0", "abc")

theStack commented at 12:58 PM on June 5, 2025:

nit: as there is already a null character inside the string, could just append a regular character at the end to cause a mismatch with this error message

        assert_raises_rpc_error(-14, "wallet passphrase entered is incorrect. It contains a null character (ie - a zero byte)", self.nodes[0].walletpassphrase, passphrase_with_nulls + "x", 10)
        assert_raises_rpc_error(-14, "The old wallet passphrase entered is incorrect. It contains a null character (ie - a zero byte)", self.nodes[0].walletpassphrasechange, passphrase_with_nulls + "x", "abc")

brunoerg commented at 2:24 PM on June 5, 2025:

Yes, make sense, thanks. I will just keep as-is to not invalidate the reviews.

theStack approved

theStack commented at 12:59 PM on June 5, 2025: contributor

ACK 7cfbb8575e1ffbad5c48e2c461b45dd6ac63d064

in test/functional/wallet_encryption.py:99 in 7cfbb8575e

  94 | @@ -95,6 +95,9 @@ def run_test(self):
  95 |          self.nodes[0].walletpassphrasechange(passphrase2, passphrase_with_nulls)
  96 |          # walletpassphrasechange should not stop at null characters
  97 |          assert_raises_rpc_error(-14, "wallet passphrase entered was incorrect", self.nodes[0].walletpassphrase, passphrase_with_nulls.partition("\0")[0], 10)
  98 | +        assert_raises_rpc_error(-14, "The wallet passphrase entered was incorrect", self.nodes[0].walletpassphrasechange, passphrase_with_nulls.partition("\0")[0], "abc")
  99 | +        assert_raises_rpc_error(-14, "wallet passphrase entered is incorrect. It contains a null character (ie - a zero byte)", self.nodes[0].walletpassphrase, passphrase_with_nulls + "\0", 10)

pablomartin4btc commented at 4:19 PM on June 5, 2025:

nit: again if you have to retouch... otherwise when you search for that "similar" description in the code you get both places ("The wallet" and "The Old wallet")

        assert_raises_rpc_error(-14, "The wallet passphrase entered is incorrect. It contains a null character (ie - a zero byte)", self.nodes[0].walletpassphrase, passphrase_with_nulls + "\0", 10)

brunoerg commented at 2:50 PM on June 6, 2025:

I will leave as-is for now to not invalidate the reivews. Thanks.

in test/functional/wallet_encryption.py:96 in 7cfbb8575e

  94 | @@ -95,6 +95,9 @@ def run_test(self):
  95 |          self.nodes[0].walletpassphrasechange(passphrase2, passphrase_with_nulls)
  96 |          # walletpassphrasechange should not stop at null characters

pablomartin4btc commented at 4:19 PM on June 5, 2025:

nit: if you have to retouch maybe... even if it's not entirely part of the scope of the change...

        # walletpassphrasechange should not stop at null characters as prior to v25.0

And a test could be added in wallet_backcompatibility.py if it makes sense.

brunoerg commented at 2:50 PM on June 6, 2025:

Same. And maybe out of scope, will see.

in test/functional/wallet_encryption.py:98 in 7cfbb8575e

  94 | @@ -95,6 +95,9 @@ def run_test(self):
  95 |          self.nodes[0].walletpassphrasechange(passphrase2, passphrase_with_nulls)
  96 |          # walletpassphrasechange should not stop at null characters
  97 |          assert_raises_rpc_error(-14, "wallet passphrase entered was incorrect", self.nodes[0].walletpassphrase, passphrase_with_nulls.partition("\0")[0], 10)
  98 | +        assert_raises_rpc_error(-14, "The wallet passphrase entered was incorrect", self.nodes[0].walletpassphrasechange, passphrase_with_nulls.partition("\0")[0], "abc")

pablomartin4btc commented at 4:23 PM on June 5, 2025:

nit: just if you retouch... to be consistent with other places where that error description is being compared...

        assert_raises_rpc_error(-14, "wallet passphrase entered was incorrect", self.nodes[0].walletpassphrasechange, passphrase_with_nulls.partition("\0")[0], "abc")

brunoerg commented at 2:51 PM on June 6, 2025:

Same

pablomartin4btc commented at 4:29 PM on June 5, 2025: member

cr ACK 7cfbb8575e1ffbad5c48e2c461b45dd6ac63d064

In the description: "This PR adds test coverage for the walletpassphrase RPC..." should include also "walletpassphrasechange".

achow101 commented at 10:05 PM on June 6, 2025: member

ACK 7cfbb8575e1ffbad5c48e2c461b45dd6ac63d064

achow101 merged this on Jun 6, 2025

achow101 closed this on Jun 6, 2025