Add a feature for serde deny unknown fields #367

jamillambert · 2025-09-18T16:23:39Z

Currently there is no way to turn off the deny unknown fields attribute. This can cause issues when using the crate in cases where there are fields returned by the RPC that are not yet in the types structs, e.g. in unmerged Core PRs (see #366).

Reorder the dependencies in the Cargo.toml files to make checking for consistency across the crates easier.
Use relative paths for the corepc crates dependencies.
Remove one case of #[serde(deny_unknown_fields)] in client that is not needed.
Add a feature that can be used to enable the attribute, which is off by default since it is only needed in testing. Enable the feature in integration_test by adding a types dependency.
Search and replace all occurrences of #[serde(deny_unknown_fields)] with #[cfg_attr(feature = "serde-deny-unknown-fields", serde(deny_unknown_fields))]

Closes #366

jamillambert · 2025-09-18T16:25:17Z

I am not 100% sure about the use of the feature in node and client, e.g. should it be default, added directly to the types dependency and always on?

tcharding · 2025-09-18T22:16:52Z

I rekon we should not enable it by default. We can instead just add the feature to types then add a dependency in integration_test that enables the feature. This makes life easier for users with no loss of test coverage.

# Just so we can enable the feature.
types = { version = "0.9.0", features = "serde-deny-unknow-fields" }

jamillambert · 2025-09-19T08:30:28Z

I updated it so that it's off by default and turned on for integration_test.

The feature is used in integration_test by enabling it in the node dependency. node then forwards to client which then forwards to types. This way it can be enabled through the existing dependency chain.

tcharding · 2025-09-21T23:26:33Z

We can do this more simply.

In 4b3d17ba just put (and this time I tested it locally)

       # Just so we can enable the feature.
       types = { package = "corepc-types", version = "0.9.0", features = ["serde-deny-unknown-fields"] }

Then add the feature in types (like already done). And no other changes.

What that means is only we test the unknown fields in integration tests (i.e., we rely on ourselves to prove they are correct).

0xB10C · 2025-09-22T07:10:01Z

I played around with this a bit to see if this works for me. It works well when I depend on corepc-types directly, but does not when I use the types via corepc-client crate (as I currently do in peer-observer). Demonstrated here: 0xB10C/corepc-pr-367-tester@0c98186

0xB10C · 2025-09-22T07:57:23Z

Ah, it's using the types pinned to 0.9.0 (with #[serde(deny_unknown_fields)]).

When using relative types, it works fine:

See 0xB10C/corepc-pr-367-tester@28674a7 and 0xB10C@7e0864d

tnull · 2025-09-22T09:13:46Z

types/Cargo.toml

 [features]
 default = ["std"]
 std = ["bitcoin/std"]
+serde-deny-unknown-fields = []


To maintain the previous default behavior, would it make sense to add this to default here and in the other crates?

We decided to have it off by default. The feature is only needed in testing, see the comment above by @tcharding.

Thanks for reviewing @tnull! Good to see you.

I don't quite follow why it's only used in tests, but also don't have a super strong opinion that it needs to be enabled by default.

I don't really have a strong opinion either and did have it on by default to start with.

The reason though is that with the feature off the decoding will pass even if there is a returned field not in the struct, which means if there are changes in Core the current functionality of types will remain the same. All the returned fields from the RPCs in their current state will still be decoded, otherwise it will stop working altogether until it is patched.

Obviously we still have the feature on in integration_test so that any changes to the returned fields will be caught in testing, which is the reason it was added in the first place.

jamillambert · 2025-09-22T17:33:20Z

Ah, it's using the types pinned to 0.9.0 (with #[serde(deny_unknown_fields)]).

When using relative types, it works fine:

See 0xB10C/corepc-pr-367-tester@28674a7 and 0xB10C@7e0864d

I added a patch at the start to change client to use a relative path for the types dependency.

jamillambert · 2025-09-22T17:36:11Z

       # Just so we can enable the feature.
       types = { package = "corepc-types", version = "0.9.0", features = ["serde-deny-unknown-fields"] }

Changed to use this instead for forwarding the feature. I also removed a use of the serde deny in client that is not needed. I suspect it was added with a repo wide search and replace when I added it the first time under all occurrences of #[derive(Clone, Debug, PartialEq, Deserialize, Serialize)].

jamillambert · 2025-09-22T17:44:36Z

Suggested changes made and the PR description updated to include the 2 new patches.

tcharding · 2025-09-22T22:07:17Z

client/Cargo.toml

 [dependencies]
 bitcoin = { version = "0.32.0", default-features = false, features = ["std", "serde"] }
-types = { package = "corepc-types", version = "0.9.0", default-features = false, features = ["std"] }
+types = { package = "corepc-types", path = "../types", default-features = false, features = ["std"] }


Can you change the deps for all crates in this repo to use path instead of version please. Its superior in all ways AFAICT, I just didn't know that when I started this repo.

Can you change the deps for all crates in this repo to use path instead of version please. Its superior in all ways AFAICT, I just didn't know that when I started this repo.

I was in the process of doing this yesterday and then saw the [patch.crates-io.corepc-node] sections in integration_test and assumed it was done for a reason.

I admit I don't entirely understand it, I thought that the patch line should override the version in the dependency and use the local copy. There is a workspace root patch line for types too. But this didn't do this for @0xB10C when using types through client.

It is all changed to use path instead. I have left fuzz untouched since it's broken and needs to be fixed separately.

tcharding · 2025-09-22T22:08:21Z

integration_test/Cargo.toml

 [dependencies]
 bitcoin = { version = "0.32.0", default-features = false, features = ["std", "serde"] }
 node = { package = "corepc-node", version = "0.9.0", default-features = false }
+types = { package = "corepc-types", version = "0.9.0", features = ["serde-deny-unknown-fields"] }


Suggested change

types = { package = "corepc-types", version = "0.9.0", features = ["serde-deny-unknown-fields"] }

# Just so we can enable the feature.

types = { package = "corepc-types", version = "0.9.0", features = ["serde-deny-unknown-fields"] }

Otherwise future Tobin or some external contributor will likely remove this dep because its not immediately obvious why its there.

jamillambert · 2025-09-23T08:04:59Z

Suggested changes made.

0xB10C · 2025-09-23T08:43:42Z

ACK f4c5c4d

Changes look good to me and work for me in my tester 0xB10C/corepc-pr-367-tester@e13a8a4

tcharding · 2025-09-23T22:27:43Z

In 6ea9bae

I'd like to keep the optional deps separate from non-optional. Same as we do in rust-bitcoin.

I'm on board with the alphabetic order if you want that, or you can put the local deps grouped first if you prefer that, or some combination of the two. So long as the optional deps are clearly separate somewhere below the non-optional deps I'm happy.

tcharding · 2025-09-23T22:29:22Z

Everything else looks good.

Thanks @0xB10C for the testing!

The dependencies in the crates were in different orders making it harder to check for consistancy whan scanning between them. Change them all to be alphabetical with the local dependencies at the end.

Using a versioned dependency for causes an issue with adding a new feature to the types crate. Use a relative path dependency instead. Remove the patch section in integration_test/Cargo.toml since it's no longer needed.

There was one use of the feature, but it is not needed since it is a struct for the input of an RPC and not the return. Remove the feature gate.

Currently there is no way to turn off the deny unknown fields attribute. This can cause issues when using the crate in cases where there are new fields added that are not yet in the types structs, e.g. in unmerged Core PRs. Add a feature to types that can be used to enable this attribute, which is off by default. Forward the feature to client and then to node. Use the feature in integration_test, through the node dependency.

There is a new feature for enabling `serde(deny_unknown_fields)` instead of having it always on. Search and replace all occurances of `#[serde(deny_unknown_fields)]` to only be enabled with the feature.

jamillambert · 2025-09-24T06:57:30Z

I'd like to keep the optional deps separate from non-optional. Same as we do in rust-bitcoin.

Done.

tcharding

ACK 14c3dd6

tcharding · 2025-09-24T09:44:06Z

Hey @0xB10C do you need a release of this?

Newer Bitcoin Core versions and e.g. Bitcoin Knots might return fields not present in the corepc-types version used. These previously caused a panic. This was fixed in rust-bitcoin/corepc#367, so use corepc master until there's a new release. Closes #260

0xB10C · 2025-09-25T15:47:18Z

Hey @0xB10C do you need a release of this?

no, happy to use master for a while, thanks for asking though!

…ields 14c3dd692c74a60b4e9f36e4bb71d111f98d7b48 Feature gate serde(deny_unknown_fields) (Jamil Lambert, PhD) ba366fdaed80f0840749c8b298d562a3b9c5769f Add a feature for serde deny unknown fields (Jamil Lambert, PhD) 2406d1219faa931eb4daceb4058d46361c6b50f0 Remove serde deny_unknown_fields from client (Jamil Lambert, PhD) ced1f4f6126974e1340b4f1e2dd22c113828b300 Use relative path dependencies for corepc crates (Jamil Lambert, PhD) f8fa8d6a69c076c2a292747a122878f0b6f06cc7 Reorder dependencies in Cargo.toml files (Jamil Lambert, PhD) Pull request description: Currently there is no way to turn off the deny unknown fields attribute. This can cause issues when using the crate in cases where there are fields returned by the RPC that are not yet in the types structs, e.g. in unmerged Core PRs (see #366). - Reorder the dependencies in the `Cargo.toml` files to make checking for consistency across the crates easier. - Use relative paths for the `corepc` crates dependencies. - Remove one case of `#[serde(deny_unknown_fields)]` in `client` that is not needed. - Add a feature that can be used to enable the attribute, which is off by default since it is only needed in testing. Enable the feature in `integration_test` by adding a `types` dependency. - Search and replace all occurrences of `#[serde(deny_unknown_fields)]` with `#[cfg_attr(feature = "serde-deny-unknown-fields", serde(deny_unknown_fields))]` Closes #366 ACKs for top commit: tcharding: ACK 14c3dd692c74a60b4e9f36e4bb71d111f98d7b48 Tree-SHA512: 519ab2218685aeedfad737339a5907793c3ce0d80d5a5a5b6c56385af924bca6b13ae94e68baeb1c7a66a4f090eac3254f6137adcbdbde2393dc0ebf5a14e440

Newer Bitcoin Core versions and e.g. Bitcoin Knots might return fields not present in the corepc-types version used. These previously caused a panic. This was fixed in rust-bitcoin/corepc#367, so use corepc master until there's a new release. Closes 0xB10C/peer-observer#260

github-actions bot added C-client C-types labels Sep 18, 2025

jamillambert force-pushed the 0918-serde-deny-feature branch from 5875d10 to d4dd790 Compare September 19, 2025 08:26

github-actions bot added C-integration-test C-node labels Sep 19, 2025

tnull reviewed Sep 22, 2025

View reviewed changes

jamillambert force-pushed the 0918-serde-deny-feature branch from d4dd790 to b80fbe1 Compare September 22, 2025 17:32

tcharding reviewed Sep 22, 2025

View reviewed changes

jamillambert force-pushed the 0918-serde-deny-feature branch from b80fbe1 to f4c5c4d Compare September 23, 2025 07:50

github-actions bot added the C-jsonrpc label Sep 23, 2025

jamillambert added 5 commits September 24, 2025 07:56

Reorder dependencies in Cargo.toml files

f8fa8d6

The dependencies in the crates were in different orders making it harder to check for consistancy whan scanning between them. Change them all to be alphabetical with the local dependencies at the end.

Use relative path dependencies for corepc crates

ced1f4f

Using a versioned dependency for causes an issue with adding a new feature to the types crate. Use a relative path dependency instead. Remove the patch section in integration_test/Cargo.toml since it's no longer needed.

Remove serde deny_unknown_fields from client

2406d12

There was one use of the feature, but it is not needed since it is a struct for the input of an RPC and not the return. Remove the feature gate.

Feature gate serde(deny_unknown_fields)

14c3dd6

There is a new feature for enabling `serde(deny_unknown_fields)` instead of having it always on. Search and replace all occurances of `#[serde(deny_unknown_fields)]` to only be enabled with the feature.

jamillambert force-pushed the 0918-serde-deny-feature branch from f4c5c4d to 14c3dd6 Compare September 24, 2025 06:56

tcharding approved these changes Sep 24, 2025

View reviewed changes

tcharding merged commit 3a9c52a into rust-bitcoin:master Sep 24, 2025
30 checks passed

0xB10C mentioned this pull request Sep 24, 2025

fix: rpc-extractor allow extra fields in RPC results 0xB10C/peer-observer#270

Merged

jamillambert mentioned this pull request Oct 10, 2025

Addversion specifier for dependencies in this repo rust-bitcoin/rust-bitcoin#5102

Open

	types = { package = "corepc-types", version = "0.9.0", features = ["serde-deny-unknown-fields"] }
	# Just so we can enable the feature.
	types = { package = "corepc-types", version = "0.9.0", features = ["serde-deny-unknown-fields"] }

Add a feature for serde deny unknown fields #367

Add a feature for serde deny unknown fields #367

Uh oh!

Conversation

jamillambert commented Sep 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jamillambert commented Sep 18, 2025

Uh oh!

tcharding commented Sep 18, 2025

Uh oh!

jamillambert commented Sep 19, 2025

Uh oh!

tcharding commented Sep 21, 2025

Uh oh!

0xB10C commented Sep 22, 2025

Uh oh!

0xB10C commented Sep 22, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamillambert commented Sep 22, 2025

Uh oh!

jamillambert commented Sep 22, 2025

Uh oh!

jamillambert commented Sep 22, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamillambert commented Sep 23, 2025

Uh oh!

0xB10C commented Sep 23, 2025

Uh oh!

tcharding commented Sep 23, 2025

Uh oh!

tcharding commented Sep 23, 2025

Uh oh!

jamillambert commented Sep 24, 2025

Uh oh!

tcharding left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tcharding commented Sep 24, 2025

Uh oh!

0xB10C commented Sep 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jamillambert commented Sep 18, 2025 •

edited

Loading

0xB10C commented Sep 25, 2025 •

edited

Loading