Fix field name styling in `submitpackage` RPC results

tnull commented at 7:40 pm on February 18, 2025: none

Previously, the submitpackage RPC call returned a response key named package_msg, which is inconsistent with the other field names that are using - as a delimiter. Here, we align the style of package-msg.

cc @glozow @instagibbs

DrahtBot commented at 7:40 pm on February 18, 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/31900.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	stickies-v

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:

#31385 (package validation: relax the package-not-child-with-unconfirmed-parents rule by glozow)

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.

glozow added the label RPC/REST/ZMQ on Feb 18, 2025

glozow commented at 7:52 pm on February 18, 2025: member

lgtm, though I’ve heard we don’t like hyphens, so perhaps go in the other direction and change all the other ones to _?

tnull commented at 8:13 pm on February 18, 2025: none

lgtm, though I’ve heard we don’t like hyphens, so perhaps go in the other direction and change all the other ones to _?

Hmm, sure, happy to change if that would be preferable, mostly went this way as I saw other examples using - (e.g., p2sh-segwit in decodescript result, bit125-replaceable in the wallet RPCs, etc), but I now realize there are also many (more?) that use _. This makes me wonder if we are really positive it’s worth breaking the current API, if neither of the options (- or _) is consistently used across the RPC API? I guess in the submitpackage case it’s a tad more awkward as both variants are used in a single RPC response?

glozow commented at 8:30 pm on February 18, 2025: member

I want to say this changed over time… I can’t find the thread where I was convinced, but here’s chat GPT’s argument for why underscores are better than hyphens in JSON:

Consistency with JavaScript & JSON standards – JSON keys are commonly accessed in JavaScript, and using hyphens (some-key) requires bracket notation (obj[“some-key”]) instead of the simpler dot notation (obj.some_key).
Better compatibility – Many programming languages (Python, JavaScript, etc.) use underscores for variable and property names, making underscore-separated keys more natural to work with.
Avoids potential parsing issues – Some parsers and serialization libraries may misinterpret hyphens as minus signs, leading to unexpected issues."

fwiw, if we think of pre-29 as the last chance to be liberal with changing API, I do think unifying it within submitpackage makes sense (for context, this is a continuation of the convo from https://github.com/spesmilo/electrum-protocol/issues/4#issuecomment-2665116596).

stickies-v commented at 11:09 am on February 19, 2025: contributor

The developer notes state:

Argument and field naming: please consider whether there is already a naming style or spelling convention in the API for the type of object in question (blockhash, for example), and if so, try to use that. If not, use snake case fee_delta (and not, e.g. feedelta or camel case feeDelta).

Rationale: Consistency with the existing interface.

So it seems like if any change is made, it should be package_msg, or update developer notes first. The JSON-RPC spec does not provide guidelines on using either or the other, and it doesn’t seem to me like there is an industry standard either, so I don’t see a particular reason to update the developer notes.

fwiw, if we think of pre-29 as the last chance to be liberal with changing API, I do think unifying it within submitpackage makes sense

I agree with that. Concept ACK.

Align `submitpackage` result field names to use snake_case

Previously, the result of a `submitpackage` RPC call would include
fields with mixed styles (mixing dash-case and snake_case). Here, we
align them all to use snake_case, i.e., to use the `_` delimiter.

7038540a57

Align `testmempoolaccept` result field names to use snake_case

Previously, the result of a `testmempoolaccept` RPC call would include
fields with mixed styles (mixing dash-case and snake_case). Here, we
align them all to use snake_case, i.e., to use the `_` delimiter.

a85ea7d1b4

tnull force-pushed on Feb 19, 2025

tnull renamed this:
~~Fix delimeter in `package-msg` field of `submitpackage` RPC~~
Fix field name styling in `submitpackage` RPC results
on Feb 19, 2025

tnull commented at 1:36 pm on February 19, 2025: none

Alright, I now updated this PR to adjust all submitpackage result field names that previously used dash-case. As it seemed a bit odd to have some of the same fields (effective-feerate, effective-includes) with different styling as part of testmempoolaccept, I now also added a commit aligning the fields there, too, but please let me know if I should simply drop that commit if we wouldn’t want to break that RPC API as well.

fwiw, if we think of pre-29 as the last chance to be liberal with changing API, I do think unifying it within submitpackage makes sense (for context, this is a continuation of the convo from spesmilo/electrum-protocol#4 (comment)).

Alright, good to know. It might be noteworthy that there are other projects that directly forward Core’s JSON (e.g. the Esplora implementations over at https://github.com/mempool/electrs/pull/105 and https://github.com/Blockstream/electrs/pull/119) that would also be affected by the API breakage (and for clients it might also be a bit annoying as they surely can’t know with version of Core a particular Esplora server is running under the hood, but I guess they’d need to figure something out, or just use the newer version asap?). That said, I agree that if this should get fixed/aligned, now might be the last chance to do so (also given that said downstream projects haven’t fully finished deploying submitpackage support yet).

stickies-v commented at 2:29 pm on February 19, 2025: contributor

submitpackage is marked as experimental and unstable, so changing that is fine imo. That does not hold for testmempoolaccept, so if we make changes to fields you’ll have to add a -deprecatedrpc option. I don’t think breaking a stable API just for stylistic purposes is worth it, so I would postpone that until we need another breaking change.

Would be good to add release notes to document the submitpackage breaking changes.

glozow added the label Needs release note on Feb 19, 2025

in src/rpc/mempool.cpp:150 in a85ea7d1b4

150                         }},
151                     }},
152-                    {RPCResult::Type::STR, "reject-reason", /*optional=*/true, "Rejection reason (only present when 'allowed' is false)"},
153-                    {RPCResult::Type::STR, "reject-details", /*optional=*/true, "Rejection details (only present when 'allowed' is false and rejection details exist)"},
154+                    {RPCResult::Type::STR, "reject_reason", /*optional=*/true, "Rejection reason (only present when 'allowed' is false)"},
155+                    {RPCResult::Type::STR, "reject_details", /*optional=*/true, "Rejection details (only present when 'allowed' is false and rejection details exist)"},

maflcko commented at 3:07 pm on February 19, 2025:

reject-details was only added in 221c789e91696569fa34dbd162d26e98cf9cab64 (master), so changing it seems fine

DrahtBot added the label CI failed on Feb 19, 2025

glozow commented at 3:17 pm on February 19, 2025: member

Alright, good to know. It might be noteworthy that there are other projects that directly forward Core’s JSON (e.g. the Esplora implementations over at https://github.com/mempool/electrs/pull/105 and https://github.com/Blockstream/electrs/pull/119) that would also be affected by the API breakage

Ah right, I forgot about that. And https://github.com/spesmilo/electrumx/pull/288 and https://github.com/spesmilo/electrum-protocol/pull/1. I think this would inconvenience a few people.

Ok… even though it’s marked as experimental, I think it’s too late to break submitpackage purely for style reasons. Fwiw I did think this was worthwhile before (and I pinged @tnull after seeing comment), but I’m leaning towards nack now. My apologies!

tnull commented at 3:43 pm on February 19, 2025: none

Ok… even though it’s marked as experimental, I think it’s too late to break submitpackage purely for style reasons. Fwiw I did think this was worthwhile before (and I pinged @tnull after seeing comment), but I’m leaning towards nack now. My apologies!

Alright, no worries! Going ahead and closing this.

tnull closed this on Feb 19, 2025

glozow removed the label Needs release note on Feb 19, 2025

Fix field name styling in submitpackage RPC results #31900

Code Coverage & Benchmarks

Reviews

Conflicts

Fix field name styling in `submitpackage` RPC results #31900