fuzz: add ConstructPubKeyBytes util function #28361

josibake · 2023-08-29T08:12:15Z

In #28246 and #28122 , we add a PubKeyDestination and a V0SilentPaymentsDestination. Both of these PRs update fuzz/util.cpp and need a way to create well-formed pubkeys. Currently in fuzz/util.cpp, we have some logic for creating pubkeys in the multisig data provider. This logic is duplicated in #28246 and duplicated again in #28122. Seems much better to have a ConstructPubKeyBytes function that both PRs (and any future work) can reuse.

This PR introduces a function to do this and has the existing code use it. While the purpose is to introduce a utility function, the previous multisig code used ConsumeIntegralInRange(4, 7) which would have created some uncompressed pubkeys with the prefix 0x05, which is incorrect (see https://bitcoin.stackexchange.com/questions/57855/c-secp256k1-what-do-prefixes-0x06-and-0x07-in-an-uncompressed-public-key-signif)

tldr; using PickValueFromArray is more correct as it limits to the set of defined prefixes for compressed and uncompressed pubkeys.

DrahtBot · 2023-08-29T08:12:18Z

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	Sjors

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:

#28241 (Silent payment index (for light wallets and consistency check) by Sjors)

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.

Sjors · 2023-08-29T09:45:08Z

~~utACK bc5b393~~

Maybe add 0x07 in a separate commit.

There's another behavior change here, probably a good one: while generating N keys, they're now completely different. Previously we would only change the last byte to make them unique.

src/test/fuzz/util.cpp

maflcko · 2023-08-29T09:51:03Z

There's another behavior change here, probably a good one: while generating N keys, they're now completely different. Previously we would only change the last byte to make them unique.

No, they are not guaranteed to. Now they will consume bytes for their whole length, which may exhaust the available bytes faster and thus result in all-zeros and also bloat the fuzz inputs git folder. Previously they consumed no additional bytes.

Not sure why this is called a refactor, when everything is changed.

Sjors · 2023-08-29T09:52:43Z

Now they will consume bytes for their whole length, which may exhaust the available bytes faster and thus result in all-zeros and also bloat the fuzz inputs git folder

Ah, that's not good.

src/test/fuzz/util.cpp

josibake · 2023-08-29T17:56:46Z

Not sure why this is called a refactor, when everything is changed.

That was an oversite on my part. I've updated the function to use the buffer as before, so the first commit should be just a refactor now. I also split out adding the missing hybrid pubkey byte into it's own commit.

josibake · 2023-08-29T18:01:59Z

Maybe add 0x07 in a separate commit.

There's another behavior change here, probably a good one: while generating N keys, they're now completely different. > Previously we would only change the last byte to make them unique.

Done! The first commit should now be strictly a refactor.

src/test/fuzz/util.cpp

josibake · 2023-08-30T08:02:20Z

CI failure seems unrelated to the changes here unless I'm missing something. @hebasto ?

Sjors · 2023-08-30T09:03:46Z

ACK ab94167

From libsecp256k1:

/** Prefix byte used to tag various encoded curvepoints for specific purposes */
#define SECP256K1_TAG_PUBKEY_EVEN 0x02
#define SECP256K1_TAG_PUBKEY_ODD 0x03
#define SECP256K1_TAG_PUBKEY_UNCOMPRESSED 0x04
#define SECP256K1_TAG_PUBKEY_HYBRID_EVEN 0x06
#define SECP256K1_TAG_PUBKEY_HYBRID_ODD 0x07

We now cover all of these.

feature_fee_estimation.py failed in the Win64 CI, which should be unrelated to the fuzzer change.

hebasto · 2023-08-30T09:05:19Z

CI failure seems unrelated to the changes here unless I'm missing something. @hebasto ?

Restarted.

josibake · 2023-08-30T11:15:12Z

CI failure seems unrelated to the changes here unless I'm missing something. @hebasto ?

Restarted.

Still failing but with a different functional test failure now, which seems to be related to timeouts.

Sjors · 2023-08-30T14:16:15Z

It's green now.

maflcko

lgtm, all good.

Though, refactor is still wrong, because the fuzz input format is changed.

Generally I don't think the refactor label makes any sense for (unit) tests, because end users in production are never affected by test-only changes.

src/test/fuzz/util.cpp

josibake · 2023-08-30T15:23:25Z

Though, refactor is still wrong, because the fuzz input format is changed.

I removed it from the title, but feel that it's still useful on the commit message. Using the refactor label, to me, is about communicating intent to the reviewer. The first commit in this PR was not meant to change behavior, and calling it a refactor made it more obvious to reviewers that some behavior had in fact changed.

Adding 0x6 is definitely not a refactor (tho still a useful change, imo), which is why I've removed refactor from the title.

src/test/fuzz/util.cpp

maflcko · 2023-08-30T15:25:52Z

src/test/fuzz/util.cpp

@@ -14,6 +14,19 @@

 #include <memory>

+std::vector<uint8_t> ConstructPubKeyBytes(FuzzedDataProvider& fuzzed_data_provider, Span<const uint8_t> byte_data, const bool compressed) noexcept


nit: Could name it pk_data over byte_data?

I prefer byte_data because we are reading in an array of bytes and turning it into pk_data by adding a prefix byte to indicate compressed or uncompressed. At the moment we read it in, it is not pk_data. We turn it into pk_data (i.e. a byte array with the correct prefix), and then that is returned and can be serialized or turned into a CPubKey object.

maflcko · 2023-08-30T15:26:39Z

Adding 0x6 is definitely not a refactor (tho still a useful change, imo), which is why I've removed refactor from the title.

6 was already handled before, no?

Today, this code only has one spot where it needs well-formed pubkeys, but future PRs will want to reuse this code. Add a function which creates a well-formed byte array that can be turned into a pubkey. It is not required that the pubkey is valid, just that it can be recognized as a compressed or uncompressed pubkey. Note: while the main intent of this commit is to wrap the existing logic into a function, it also switches to `PickValueFromArray` so that we are only choosing one of 0x04, 0x06, or 0x07. The previous code, `ConsumeIntegralInRange` would have also picked 0x05, which is not definied in the context of compressed vs uncompressed keys. See https://bitcoin.stackexchange.com/questions/57855/c-secp256k1-what-do-prefixes-0x06-and-0x07-in-an-uncompressed-public-key-signif for more details.

josibake · 2023-08-30T15:57:07Z

@MarcoFalke squashed and removed the refactor label since we will no longer be creating pubkeys with an 0x05 prefix after this change.

maflcko

Seems fine to remove 5. The reason to have it was to also create "invalid" data and see if it causes a crash. But no strong opinion, as it may be covered by other fuzz tests already.

josibake · 2023-08-30T16:23:42Z

The reason to have it was to also create "invalid" data and see if it causes a crash

That doesn't match the comment, // Set first byte for GetLen() to pass, so if that was the intention for this specific test case, we should probably add a step that randomly mutates the bytes by adding an invalid prefix or just reads from the byte buffer without using the function.

Going forward, I think it's better to have a function just for creating well-formed pubkey data (i.e. correct prefixes) and then use random or invalid data for cases where we actually want to test for that.

Sjors · 2023-08-31T08:11:06Z

Going forward, I think it's better to have a function just for creating well-formed pubkey data (i.e. correct prefixes) and then use random or invalid data for cases where we actually want to test for that.

That makes sense, but since we previously covered 0x05 (if only by accident), the ConsumeScript function should occasionally set 0x05 (or some other invalid value) as the prefix.

maflcko · 2023-08-31T10:42:35Z

In theory malformed data can also be covered by the outer wrapper, see

                [&] {
                    // Insert byte vector directly to allow malformed or unparsable scripts
                    r_script.insert(r_script.end(), buffer.begin(), buffer.begin() + fuzzed_data_provider.ConsumeIntegralInRange(0U, MAX_BUFFER_SZ));
                },

Absent data, I think anything is fine here. I just wanted to clarify that what you are changing is intentional.

josibake · 2023-08-31T11:07:03Z

That makes sense, but since we previously covered 0x05 (if only by accident), the ConsumeScript function should occasionally set 0x05 (or some other invalid value) as the prefix.

I'm inclined to leave this pull as is, and if we find there is a real need for mutating with an invalid prefix inside the "push multisig" case, we address it in a follow-up. The way things are written now seems more clear to me: we have a case for sending random bytes to r_script and we have a case for creating a well-formed but not necessarily valid multisig script. This seems to better match the intent of the ConsumeScript function, based on the comments and surrounding code.

josibake · 2023-08-31T12:50:44Z

@MarcoFalke @Sjors can you let me know if this looks good as-is (where some things can be addressed in follow-ups), or if you'd like to see more changes in this PR before merging?

Sjors · 2023-08-31T13:07:04Z

Nothing blocking, but haven't re-reviewed the code yet.

josibake · 2023-09-07T07:39:51Z

ping @Sjors @MarcoFalke

Sjors · 2023-09-07T14:33:29Z

ACK 1580e3b

1580e3b fuzz: add ConstructPubKeyBytes function (josibake) Pull request description: In bitcoin#28246 and bitcoin#28122 , we add a `PubKeyDestination` and a `V0SilentPaymentsDestination`. Both of these PRs update `fuzz/util.cpp` and need a way to create well-formed pubkeys. Currently in `fuzz/util.cpp`, we have some logic for creating pubkeys in the multisig data provider. This logic is duplicated in bitcoin#28246 and duplicated again in bitcoin#28122. Seems much better to have a `ConstructPubKeyBytes` function that both PRs (and any future work) can reuse. This PR introduces a function to do this and has the existing code use it. While the purpose is to introduce a utility function, the previous multisig code used `ConsumeIntegralInRange(4, 7)` which would have created some uncompressed pubkeys with the prefix 0x05, which is incorrect (see https://bitcoin.stackexchange.com/questions/57855/c-secp256k1-what-do-prefixes-0x06-and-0x07-in-an-uncompressed-public-key-signif) tldr; using `PickValueFromArray` is more correct as it limits to the set of defined prefixes for compressed and uncompressed pubkeys. ACKs for top commit: Sjors: ACK 1580e3b Tree-SHA512: c87c8bcd1f6b3a97ef772be93102efb912811c59f32211cfd531a116f1da8a57c8c6ff106b34f2a2b88d8b34fb5bc30d9f9ed6d2720113ffcaaa2f8d5dc9eb27

josibake mentioned this pull request Aug 29, 2023

wallet: Use CTxDestination in CRecipient instead of just scriptPubKey #28246

Merged

Sjors reviewed Aug 29, 2023

View reviewed changes

src/test/fuzz/util.cpp Outdated Show resolved Hide resolved

Sjors reviewed Aug 29, 2023

View reviewed changes

src/test/fuzz/util.cpp Outdated Show resolved Hide resolved

josibake force-pushed the refactor-fuzzer-util branch from bc5b393 to f4a51ac Compare August 29, 2023 17:53

josibake changed the title ~~fuzz, refactor: add ConsumePubKey util function~~ fuzz, refactor: add ConstructPubKeyBytes util function Aug 29, 2023

maflcko reviewed Aug 29, 2023

View reviewed changes

src/test/fuzz/util.cpp Outdated Show resolved Hide resolved

This was referenced Aug 29, 2023

Silent payment index (for light wallets and consistency check) #28241

Closed

Silent Payments: Implement BIP352 #28122

Draft

josibake force-pushed the refactor-fuzzer-util branch from f4a51ac to ab94167 Compare August 30, 2023 06:18

maflcko approved these changes Aug 30, 2023

View reviewed changes

src/test/fuzz/util.cpp Outdated Show resolved Hide resolved

josibake force-pushed the refactor-fuzzer-util branch from ab94167 to 9bb2627 Compare August 30, 2023 15:16

josibake changed the title ~~fuzz, refactor: add ConstructPubKeyBytes util function~~ fuzz: add ConstructPubKeyBytes util function Aug 30, 2023

DrahtBot added the Tests label Aug 30, 2023

maflcko approved these changes Aug 30, 2023

View reviewed changes

josibake force-pushed the refactor-fuzzer-util branch from 9bb2627 to 1580e3b Compare August 30, 2023 15:53

maflcko approved these changes Aug 30, 2023

View reviewed changes

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

fanquake merged commit 238d29a into bitcoin:master Sep 7, 2023

josibake deleted the refactor-fuzzer-util branch January 26, 2024 10:50

bitcoin locked and limited conversation to collaborators Jan 25, 2025

		@@ -14,6 +14,19 @@

		#include <memory>

		std::vector<uint8_t> ConstructPubKeyBytes(FuzzedDataProvider& fuzzed_data_provider, Span<const uint8_t> byte_data, const bool compressed) noexcept

fuzz: add ConstructPubKeyBytes util function #28361

fuzz: add ConstructPubKeyBytes util function #28361

Uh oh!

Conversation

josibake commented Aug 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Aug 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

Sjors commented Aug 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

maflcko commented Aug 29, 2023

Uh oh!

Sjors commented Aug 29, 2023

Uh oh!

Uh oh!

josibake commented Aug 29, 2023

Uh oh!

josibake commented Aug 29, 2023

Uh oh!

Uh oh!

josibake commented Aug 30, 2023

Uh oh!

Sjors commented Aug 30, 2023

Uh oh!

hebasto commented Aug 30, 2023

Uh oh!

josibake commented Aug 30, 2023

Uh oh!

Sjors commented Aug 30, 2023

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

josibake commented Aug 30, 2023

Uh oh!

Uh oh!

maflcko Aug 30, 2023

Choose a reason for hiding this comment

Uh oh!

josibake Aug 30, 2023

Choose a reason for hiding this comment

Uh oh!

maflcko commented Aug 30, 2023

Uh oh!

josibake commented Aug 30, 2023

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

josibake commented Aug 30, 2023

Uh oh!

Sjors commented Aug 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Aug 31, 2023

Uh oh!

josibake commented Aug 31, 2023

Uh oh!

josibake commented Aug 31, 2023

Uh oh!

Sjors commented Aug 31, 2023

Uh oh!

josibake commented Sep 7, 2023

Uh oh!

Sjors commented Sep 7, 2023

Uh oh!

Uh oh!

josibake commented Aug 29, 2023 •

edited

Loading

DrahtBot commented Aug 29, 2023 •

edited

Loading

Sjors commented Aug 29, 2023 •

edited

Loading

Sjors commented Aug 31, 2023 •

edited

Loading