doc: add guidance for RPC to developer notes #30142

tdb3 · 2024-05-19T17:24:51Z

Adds guidance statements to the RPC interface section of the developer notes with examples of when to implement -deprecatedrpc=.

Wanted to increase awareness of preferred RPC implementation approaches for newer contributors.

This implements some of what's discussed in #29912 (comment)

Opinions may differ, so please don't be shy. We want to make RPC as solid/safe as possible.

Examples of discussions where guidelines/context might have added value:
#30212 (comment)
#29845 (comment)
#30381 (review)
#29954 (comment)
#30410 (review)
#30713
#30381
#29060 (review)

DrahtBot · 2024-05-19T17:24:54Z

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/30142.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	l0rinc, fjahr, maflcko, jonatack
Concept ACK	stickies-v, polespinasa
Stale ACK	epiccurious, ismaelsadeeq

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

tdb3 · 2024-05-19T17:32:11Z

~~lint error is unrelated. Already discussed in #30084 (comment) and #30106.~~
The warning is unrelated. I had overlooked the lint error and it is now fixed.

doc/developer-notes.md

DrahtBot · 2024-05-19T18:15:16Z

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

_{Debug: https://github.com/bitcoin/bitcoin/runs/25152887143}

doc/developer-notes.md

epiccurious · 2024-05-25T12:32:45Z

doc/developer-notes.md

@@ -1449,6 +1449,16 @@ A few guidelines for introducing and reviewing new RPC interfaces:
  - *Rationale*: JSON strings are Unicode strings, not byte strings, and
    RFC8259 requires JSON to be encoded as UTF-8.

+- When choosing data types for RPC JSON, prefer data types that are flexible to expansion without change of data type. For example, `{"result":{"warnings":[]}}` rather than `{"result":{"warnings":""}}`, which allows for multiple warnings to be included as array elements.


nit: this sentence might be better structured starting with a verb:

Prefer to choose RPC JSON data types that are flexible to expansion without change of data type.

epiccurious · 2024-05-25T12:38:53Z

ACK 61853a2.

doc/developer-notes.md

stickies-v

Concept ACK

stickies-v · 2024-10-15T15:00:28Z

doc/developer-notes.md

@@ -1449,6 +1449,12 @@ A few guidelines for introducing and reviewing new RPC interfaces:
  - *Rationale*: JSON strings are Unicode strings, not byte strings, and
    RFC8259 requires JSON to be encoded as UTF-8.

+A few guidelines for modifying and reviewing existing RPC interfaces:
+
+- Implement an associated `-deprecatedrpc=` option to retain previous RPC behavior when modifying the structure of RPC interface JSON.  This includes, but is not limited to, changes such as: data types changes (e.g. from `{"result":{"warnings":""}}` to `{"result":{"warnings":[]}}`, changing a value from a string to a number, etc.), logical meaning changes of a value, or key name changes (e.g. `{"result":{"warning":""}}` to `{"result":{"warnings":""}}`).  Include detailed instructions on feature deprecation and re-enabling previous behavior in release notes.


Perhaps useful to elaborate a bit more on distinguishing between backwards compatible/incompatible behaviour?

Suggested change

- Implement an associated `-deprecatedrpc=` option to retain previous RPC behavior when modifying the structure of RPC interface JSON. This includes, but is not limited to, changes such as: data types changes (e.g. from `{"result":{"warnings":""}}` to `{"result":{"warnings":[]}}`, changing a value from a string to a number, etc.), logical meaning changes of a value, or key name changes (e.g. `{"result":{"warning":""}}` to `{"result":{"warnings":""}}`). Include detailed instructions on feature deprecation and re-enabling previous behavior in release notes.

- Implement an associated `-deprecatedrpc=` option to retain previous RPC behavior when modifying the structure of RPC interface JSON in a backwards-incompatible manner. This includes, but is not limited to, changes such as: data types changes (e.g. from `{"result":{"warnings":""}}` to `{"result":{"warnings":[]}}`, changing a value from a string to a number, etc.), logical meaning changes of a value, or key name changes (e.g. `{"result":{"warning":""}}` to `{"result":{"warnings":""}}`). Include detailed instructions on feature deprecation and re-enabling previous behavior in release notes. Adding fields is generally considered backwards compatible.

Oh would also add that deprecated behaviour should be documented in the RPC docs, such as e.g. in

bitcoin/src/rpc/net.cpp

Lines 663 to 664 in 0ca1d1b

RPCResult{RPCResult::Type::STR, "warnings", "any network and blockchain warnings (DEPRECATED)"} :

RPCResult{RPCResult::Type::ARR, "warnings", "any network and blockchain warnings (run with `-deprecatedrpc=warnings` to return the latest warning as a single string)",

?

Yes, we've also traditionally mentioned these in all-caps in the RPC helps, i.e.

RPCResult{RPCResult::Type::STR, "warnings", "(DEPRECATED) network and blockchain warnings, if any...)"} :

Perhaps for this case an example on its own line would be helpful.

Thanks. Mentioned that adding a key to an object is considered backward compatible. Also added an example from RPC code with DEPRECATED.

jonatack · 2024-10-15T15:13:19Z

doc/developer-notes.md

@@ -1449,6 +1449,12 @@ A few guidelines for introducing and reviewing new RPC interfaces:
  - *Rationale*: JSON strings are Unicode strings, not byte strings, and
    RFC8259 requires JSON to be encoded as UTF-8.

+A few guidelines for modifying and reviewing existing RPC interfaces:
+
+- Implement an associated `-deprecatedrpc=` option to retain previous RPC behavior when modifying the structure of RPC interface JSON.  This includes, but is not limited to, changes such as: data types changes (e.g. from `{"result":{"warnings":""}}` to `{"result":{"warnings":[]}}`, changing a value from a string to a number, etc.), logical meaning changes of a value, or key name changes (e.g. `{"result":{"warning":""}}` to `{"result":{"warnings":""}}`).  Include detailed instructions on feature deprecation and re-enabling previous behavior in release notes.


Include detailed instructions on feature deprecation and re-enabling previous behavior in release notes.

Release notes should refer to the RPC help for details instead of substituting for full documentation. Example: "Please refer to the RPC help of getbalances for details."

Implement an associated -deprecatedrpc= option to retain previous RPC behavior when modifying the structure of RPC interface JSON in a backwards-incompatible manner.

Suggest to lead with what would require this, use simpler language, and mention the period:

"If an RPC will be changed in a backward-incompatible way, add an associated -deprecatedrpc= option to retain previous RPC behavior during the deprecation period."

This includes, but is not limited to, ...

(Is this long sentence and its examples and legalese all necessary? Don't hesitate to shorten where possible.)

Thanks. Specified that the release note should refer to RPC help, and led with is being changed in a backwards-incompatible way

maflcko · 2025-01-22T19:36:26Z

doc/developer-notes.md

@@ -1460,6 +1460,22 @@ A few guidelines for introducing and reviewing new RPC interfaces:
  - *Rationale*: JSON strings are Unicode strings, not byte strings, and
    RFC8259 requires JSON to be encoded as UTF-8.

+A few guidelines for modifying and reviewing existing RPC interfaces:
+
+- If an RPC is being changed in a backward-incompatible way, add an associated `-deprecatedrpc=` option to retain previous RPC behavior during the deprecation period. Backward-incompatible changes include: data types changes (e.g. from `{"result":{"warnings":""}}` to `{"result":{"warnings":[]}}`, changing a value from a string to a number, etc.), logical meaning changes of a value, or key name changes (e.g. `{"result":{"warning":""}}` to `{"result":{"warnings":""}}`). Adding a key to an object is generally considered backwards compatible. A release note should be included that refers to RPC help for details of feature deprecation and re-enabling previous behavior. Example RPC help:


Could drop the redundant/repeated "result" everywhere?

Suggested change

- If an RPC is being changed in a backward-incompatible way, add an associated `-deprecatedrpc=` option to retain previous RPC behavior during the deprecation period. Backward-incompatible changes include: data types changes (e.g. from `{"result":{"warnings":""}}` to `{"result":{"warnings":[]}}`, changing a value from a string to a number, etc.), logical meaning changes of a value, or key name changes (e.g. `{"result":{"warning":""}}` to `{"result":{"warnings":""}}`). Adding a key to an object is generally considered backwards compatible. A release note should be included that refers to RPC help for details of feature deprecation and re-enabling previous behavior. Example RPC help:

- If an RPC is being changed in a backward-incompatible way, add an associated `-deprecatedrpc=` option to retain previous RPC behavior during the deprecation period. Backward-incompatible changes include: data types changes (e.g. from `{"warnings":""}` to `{"result":{"warnings":[]}}`, changing a value from a string to a number, etc.), logical meaning changes of a value, or key name changes (e.g. `{"result":{"warning":""}}` to `{"result":{"warnings":""}}`). Adding a key to an object is generally considered backwards compatible. A release note should be included that refers to RPC help for details of feature deprecation and re-enabling previous behavior. Example RPC help:

Good point. Implemented.

l0rinc

No opinion about the content, reviewed only grammatically

doc/developer-notes.md

l0rinc · 2025-01-23T09:22:57Z

doc/developer-notes.md

+A few guidelines for modifying and reviewing existing RPC interfaces:
+
+- If an RPC is being changed in a backward-incompatible way, add an associated `-deprecatedrpc=` option to retain previous RPC behavior during the deprecation period. Backward-incompatible changes include: data types changes (e.g. from `{"warnings":""}` to `{"warnings":[]}`, changing a value from a string to a number, etc.), logical meaning changes of a value, or key name changes (e.g. `{"warning":""}` to `{"warnings":""}`). Adding a key to an object is generally considered backwards compatible. A release note should be included that refers to RPC help for details of feature deprecation and re-enabling previous behavior. Example RPC help:
+    ```c++


Would it make sense to refer to the code instead, to avoid duplication and maintenance burden?

Thanks. Replaced with a permalink for it

bitcoin/src/rpc/blockchain.cpp

Lines 1316 to 1323 in 94f0adc

(IsDeprecatedRPCEnabled("warnings") ?

RPCResult{RPCResult::Type::STR, "warnings", "any network and blockchain warnings (DEPRECATED)"} :

RPCResult{RPCResult::Type::ARR, "warnings", "any network and blockchain warnings (run with `-deprecatedrpc=warnings` to return the latest warning as a single string)",

{

{RPCResult::Type::STR, "", "warning"},

}

}

),

doc/developer-notes.md

l0rinc

No opinion about the content, reviewed only grammatically

l0rinc · 2025-01-23T12:39:10Z

utACK 40ac07e

fjahr

ACK 40ac07e

Good addition!

fjahr · 2025-03-12T16:01:01Z

doc/developer-notes.md

@@ -1460,6 +1460,12 @@ A few guidelines for introducing and reviewing new RPC interfaces:
  - *Rationale*: JSON strings are Unicode strings, not byte strings, and
    RFC8259 requires JSON to be encoded as UTF-8.

+A few guidelines for modifying and reviewing existing RPC interfaces:


nit: Below doesn't talk about reviewing afaict

Suggested change

A few guidelines for modifying and reviewing existing RPC interfaces:

A few guidelines for modifying existing RPC interfaces:

True, though i suppose the same guidelines for modifying RPC interfaces can be used to review the changes by seeing if the changes follow the guidelines, without mentioning review explicitly.

Updated. Thanks.

polespinasa

Concept ACK.

I would also add some indications on how to deprecate RPCs (#31980).

See #31278 as an example of a try to deprecate an RPC and the discussions held about the procedure.

ismaelsadeeq

ACK 40ac07e

jonatack · 2025-03-13T15:11:59Z

doc/developer-notes.md

@@ -1460,6 +1460,12 @@ A few guidelines for introducing and reviewing new RPC interfaces:
  - *Rationale*: JSON strings are Unicode strings, not byte strings, and
    RFC8259 requires JSON to be encoded as UTF-8.

+A few guidelines for modifying and reviewing existing RPC interfaces:
+
+- If an RPC is being changed in a backward-incompatible way, add an associated `-deprecatedrpc=` option to retain previous RPC behavior during the deprecation period. Backward-incompatible changes include: data type changes (e.g. from `{"warnings":""}` to `{"warnings":[]}`, changing a value from a string to a number, etc.), logical meaning changes of a value, or key name changes (e.g. `{"warning":""}` to `{"warnings":""}`). Adding a key to an object is generally considered backward compatible. A release note should be included that refers to RPC help for details of feature deprecation and re-enabling previous behavior. [Example RPC help](https://github.com/bitcoin/bitcoin/blob/94f0adcc31d2d263a03e46192922e3a7672e39d9/src/rpc/blockchain.cpp#L1316-L1323).


Suggested change

- If an RPC is being changed in a backward-incompatible way, add an associated `-deprecatedrpc=` option to retain previous RPC behavior during the deprecation period. Backward-incompatible changes include: data type changes (e.g. from `{"warnings":""}` to `{"warnings":[]}`, changing a value from a string to a number, etc.), logical meaning changes of a value, or key name changes (e.g. `{"warning":""}` to `{"warnings":""}`). Adding a key to an object is generally considered backward compatible. A release note should be included that refers to RPC help for details of feature deprecation and re-enabling previous behavior. [Example RPC help](https://github.com/bitcoin/bitcoin/blob/94f0adcc31d2d263a03e46192922e3a7672e39d9/src/rpc/blockchain.cpp#L1316-L1323).

- It's preferable to avoid changing an RPC in a backward-incompatible manner, but in that case, add an associated `-deprecatedrpc=` option to retain previous RPC behavior during the deprecation period. Backward-incompatible changes include: data type changes (e.g. from `{"warnings":""}` to `{"warnings":[]}`, changing a value from a string to a number, etc.), logical meaning changes of a value, or key name changes (e.g. `{"warning":""}` to `{"warnings":""}`). Adding a key to an object is generally considered backward-compatible. Include a release note that refers the user to the RPC help for details of feature deprecation and re-enabling previous behavior. [Example RPC help](https://github.com/bitcoin/bitcoin/blob/94f0adcc/src/rpc/blockchain.cpp#L1316-L1323).

(I don't think the link is the best example, but in any case the URL can be shorter for people who read these notes in their text editor instead of online.)

Updated. Thanks.

Related to #30142 (comment) - it's more uniform now

Adds deprecatedrpc guidance statement to the RPC interface guidelines section.

l0rinc · 2025-03-20T09:20:50Z

ACK c6eca6f

fjahr · 2025-03-20T14:28:22Z

ACK c6eca6f

maflcko · 2025-03-20T14:32:48Z

lgtm ACK c6eca6f

jonatack · 2025-03-20T17:26:22Z

ACK c6eca6f

jonatack · 2025-03-20T17:29:59Z

(Meta: the current GitHub safetyism and friction is annoying, when all you want to do is ACK 😄)

"This issue is highly active. Reconsider commenting unless you have read all the comments and have something to add."

"Your link may be misinterpreted by GitHub."

DrahtBot added the Docs label May 19, 2024

jonatack reviewed May 19, 2024

View reviewed changes

doc/developer-notes.md Outdated Show resolved Hide resolved

tdb3 force-pushed the rpc_update_guidance branch from f021724 to 61853a2 Compare May 19, 2024 18:15

DrahtBot added the CI failed label May 19, 2024

DrahtBot removed the CI failed label May 19, 2024

epiccurious approved these changes May 25, 2024

View reviewed changes

maflcko reviewed May 26, 2024

View reviewed changes

doc/developer-notes.md Outdated Show resolved Hide resolved

DrahtBot added CI failed and removed CI failed labels May 26, 2024

tdb3 force-pushed the rpc_update_guidance branch from 61853a2 to b5f62f3 Compare June 3, 2024 00:41

DrahtBot added CI failed and removed CI failed labels Jun 5, 2024

achow101 requested review from laanwj and stickies-v October 15, 2024 14:55

stickies-v reviewed Oct 15, 2024

View reviewed changes

jonatack reviewed Oct 15, 2024

View reviewed changes

tdb3 force-pushed the rpc_update_guidance branch from b5f62f3 to 7b55357 Compare October 28, 2024 22:08

maflcko reviewed Jan 22, 2025

View reviewed changes

tdb3 force-pushed the rpc_update_guidance branch from 7b55357 to 0e7926e Compare January 23, 2025 01:00

l0rinc suggested changes Jan 23, 2025

View reviewed changes

tdb3 force-pushed the rpc_update_guidance branch from 0e7926e to 40ac07e Compare January 23, 2025 12:30

DrahtBot requested review from epiccurious and stickies-v January 23, 2025 12:39

ismaelsadeeq mentioned this pull request Jan 24, 2025

wallet, rpc: deprecate settxfee and paytxfee #31278

Merged

maflcko mentioned this pull request Mar 12, 2025

Doc: Feature deprecation and removal process documentation. #31980

Open

fjahr reviewed Mar 12, 2025

View reviewed changes

polespinasa reviewed Mar 12, 2025

View reviewed changes

ismaelsadeeq reviewed Mar 13, 2025

View reviewed changes

DrahtBot requested a review from polespinasa March 13, 2025 13:44

jonatack reviewed Mar 13, 2025

View reviewed changes

doc: add guidance for RPC to developer notes

c6eca6f

Adds deprecatedrpc guidance statement to the RPC interface guidelines section.

tdb3 force-pushed the rpc_update_guidance branch from 40ac07e to c6eca6f Compare March 19, 2025 22:59

DrahtBot requested review from fjahr and ismaelsadeeq March 20, 2025 09:20

fanquake merged commit b43cfa2 into bitcoin:master Mar 21, 2025
18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: add guidance for RPC to developer notes #30142

doc: add guidance for RPC to developer notes #30142

tdb3 commented May 19, 2024 •

edited

Loading

DrahtBot commented May 19, 2024 •

edited

Loading

tdb3 commented May 19, 2024 •

edited

Loading

DrahtBot commented May 19, 2024

epiccurious May 25, 2024

epiccurious commented May 25, 2024

stickies-v left a comment

stickies-v Oct 15, 2024

stickies-v Oct 15, 2024

jonatack Oct 15, 2024 •

edited

Loading

tdb3 Jan 23, 2025

jonatack Oct 15, 2024

jonatack Oct 15, 2024

tdb3 Jan 23, 2025

maflcko Jan 22, 2025

tdb3 Jan 23, 2025

l0rinc left a comment

l0rinc Jan 23, 2025

tdb3 Jan 23, 2025

l0rinc left a comment

l0rinc commented Jan 23, 2025

fjahr left a comment

fjahr Mar 12, 2025

laanwj Mar 19, 2025

tdb3 Mar 19, 2025

polespinasa left a comment

ismaelsadeeq left a comment

jonatack Mar 13, 2025

tdb3 Mar 19, 2025

l0rinc Mar 20, 2025

l0rinc commented Mar 20, 2025

fjahr commented Mar 20, 2025

maflcko commented Mar 20, 2025

jonatack commented Mar 20, 2025

jonatack commented Mar 20, 2025

	RPCResult{RPCResult::Type::STR, "warnings", "any network and blockchain warnings (DEPRECATED)"} :
	RPCResult{RPCResult::Type::ARR, "warnings", "any network and blockchain warnings (run with `-deprecatedrpc=warnings` to return the latest warning as a single string)",

	(IsDeprecatedRPCEnabled("warnings") ?
	RPCResult{RPCResult::Type::STR, "warnings", "any network and blockchain warnings (DEPRECATED)"} :
	RPCResult{RPCResult::Type::ARR, "warnings", "any network and blockchain warnings (run with `-deprecatedrpc=warnings` to return the latest warning as a single string)",
	{
	{RPCResult::Type::STR, "", "warning"},
	}
	}
	),

	A few guidelines for modifying and reviewing existing RPC interfaces:
	A few guidelines for modifying existing RPC interfaces:

doc: add guidance for RPC to developer notes #30142

doc: add guidance for RPC to developer notes #30142

Conversation

tdb3 commented May 19, 2024 • edited Loading

DrahtBot commented May 19, 2024 • edited Loading

Code Coverage & Benchmarks

Reviews

tdb3 commented May 19, 2024 • edited Loading

DrahtBot commented May 19, 2024

Choose a reason for hiding this comment

epiccurious commented May 25, 2024

stickies-v left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonatack Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

l0rinc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

l0rinc left a comment

Choose a reason for hiding this comment

l0rinc commented Jan 23, 2025

fjahr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

polespinasa left a comment

Choose a reason for hiding this comment

ismaelsadeeq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

l0rinc commented Mar 20, 2025

fjahr commented Mar 20, 2025

maflcko commented Mar 20, 2025

jonatack commented Mar 20, 2025

jonatack commented Mar 20, 2025

tdb3 commented May 19, 2024 •

edited

Loading

DrahtBot commented May 19, 2024 •

edited

Loading

tdb3 commented May 19, 2024 •

edited

Loading

jonatack Oct 15, 2024 •

edited

Loading