doc: add guidance for RPC to developer notes #30142

pull tdb3 wants to merge 1 commits into bitcoin:master from tdb3:rpc_update_guidance changing 1 files +6 −0
  1. tdb3 commented at 5:24 pm on May 19, 2024: contributor

    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 (review) #30381#pullrequestreview-2160865613 #29954 (comment) #30410#pullrequestreview-2167870869 #30713 #30381 #29060#pullrequestreview-2406688998

  2. DrahtBot commented at 5:24 pm on May 19, 2024: 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/30142.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK l0rinc
    Concept ACK stickies-v
    Stale ACK epiccurious

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

  3. DrahtBot added the label Docs on May 19, 2024
  4. tdb3 commented at 5:32 pm on May 19, 2024: contributor
    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.
  5. in doc/developer-notes.md:1455 in f0217241f8 outdated 
    1448@@ -1449,6 +1449,16 @@ A few guidelines for introducing and reviewing new RPC interfaces:
1449   - *Rationale*: JSON strings are Unicode strings, not byte strings, and
1450     RFC8259 requires JSON to be encoded as UTF-8.
1451 
1452+- 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.
1453+
1454+  - *Rationale*: Planning for expansion can reduce RPC interface updates, reducing incompatibility risk to downstream applications.
1455+
    jonatack commented at 5:40 pm on May 19, 2024:
    The extra whitespace here is the cause of the linter CI failure.
    tdb3 commented at 6:18 pm on May 19, 2024:
    Thanks. Fixed
  6. tdb3 force-pushed on May 19, 2024
  7. DrahtBot added the label CI failed on May 19, 2024
  8. DrahtBot commented at 6:15 pm on May 19, 2024: contributor

    🚧 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

  9. DrahtBot removed the label CI failed on May 19, 2024
  10. in doc/developer-notes.md:1458 in 61853a25bb outdated 
    1453+
1454+  - *Rationale*: Planning for expansion can reduce RPC interface updates, reducing incompatibility risk to downstream applications.
1455+
1456+A few guidelines for modifying and reviewing existing RPC interfaces:
1457+
1458+- When modifying RPC interface JSON structure, implement associated `-deprecatedrpc=` option to retain previous RPC behavior, including (but not limited to) the following: 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, key name changes (e.g. `{"result":{"warning":""}}` to `{"result":{"warnings":""}}`).  Include detailed instructions on feature deprecation and re-enabling in release notes.
    epiccurious commented at 12:31 pm on May 25, 2024:

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

    0Implement associated `-deprecatedrpc=` option to retain previous RPC behavior when modifying RPC interface JSON structure, including (but not limited to) the following:
    tdb3 commented at 0:41 am on June 3, 2024:
    Thanks. Agreed. Updated.
  11. in doc/developer-notes.md:1452 in 61853a25bb outdated 
    1448@@ -1449,6 +1449,16 @@ A few guidelines for introducing and reviewing new RPC interfaces:
1449   - *Rationale*: JSON strings are Unicode strings, not byte strings, and
1450     RFC8259 requires JSON to be encoded as UTF-8.
1451 
1452+- 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.
    epiccurious commented at 12:32 pm on May 25, 2024:

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

    0Prefer to choose RPC JSON data types that are flexible to expansion without change of data type.
    maflcko commented at 8:55 am on May 26, 2024:

    how is this different from “- Try to make the RPC response a JSON object.”?

    The "warnings" example here doesn’t make much sense, because both are equally flexible to expansion. (See the plural s.) The reason why array should be preferred over string is that array is the correct type to represent an array. (But that is obvious, isn’t it?)

    tdb3 commented at 0:40 am on June 3, 2024:
    Thanks for taking a look.
    Good point. Removed this statement as it is quite similar to Try to make the RPC response a JSON object and could be confusing.
  12. epiccurious approved
  13. epiccurious commented at 12:38 pm on May 25, 2024: contributor
    ACK 61853a25bbbdb6d5dfb2f1570e41f541a4b7de78.
  14. DrahtBot added the label CI failed on May 26, 2024
  15. DrahtBot removed the label CI failed on May 29, 2024
  16. tdb3 force-pushed on Jun 3, 2024
  17. DrahtBot added the label CI failed on Jun 5, 2024
  18. DrahtBot removed the label CI failed on Jun 6, 2024
  19. achow101 requested review from laanwj on Oct 15, 2024
  20. achow101 requested review from stickies-v on Oct 15, 2024
  21. in doc/developer-notes.md:1454 in b5f62f3f7d outdated 
    1448@@ -1449,6 +1449,12 @@ A few guidelines for introducing and reviewing new RPC interfaces:
1449   - *Rationale*: JSON strings are Unicode strings, not byte strings, and
1450     RFC8259 requires JSON to be encoded as UTF-8.
1451 
1452+A few guidelines for modifying and reviewing existing RPC interfaces:
1453+
1454+- 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.
    stickies-v commented at 3:00 pm on October 15, 2024:

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

    0- 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.
    stickies-v commented at 3:01 pm on October 15, 2024:
    Oh would also add that deprecated behaviour should be documented in the RPC docs, such as e.g. in https://github.com/bitcoin/bitcoin/blob/0ca1d1bf69ca364393e924cf41becfde1b68126c/src/rpc/net.cpp#L663-L664?
    jonatack commented at 3:13 pm on October 15, 2024:

    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.”

    jonatack commented at 3:20 pm on October 15, 2024:

    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.)

    jonatack commented at 3:26 pm on October 15, 2024:

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

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

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

    tdb3 commented at 1:05 am on January 23, 2025:
    Thanks. Specified that the release note should refer to RPC help, and led with is being changed in a backwards-incompatible way
    tdb3 commented at 1:08 am on January 23, 2025:
    Thanks. Mentioned that adding a key to an object is considered backward compatible. Also added an example from RPC code with DEPRECATED.
  22. stickies-v commented at 3:01 pm on October 15, 2024: contributor
    Concept ACK
  23. tdb3 force-pushed on Oct 28, 2024
  24. in doc/developer-notes.md:1465 in 7b55357d5e outdated 
    1459@@ -1460,6 +1460,22 @@ A few guidelines for introducing and reviewing new RPC interfaces:
1460   - *Rationale*: JSON strings are Unicode strings, not byte strings, and
1461     RFC8259 requires JSON to be encoded as UTF-8.
1462 
1463+A few guidelines for modifying and reviewing existing RPC interfaces:
1464+
1465+- 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:
    maflcko commented at 7:36 pm on January 22, 2025:

    Could drop the redundant/repeated “result” everywhere?

    0- 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:
    tdb3 commented at 1:01 am on January 23, 2025:
    Good point. Implemented.
  25. tdb3 force-pushed on Jan 23, 2025
  26. in doc/developer-notes.md:1465 in 0e7926e68e outdated 
    1459@@ -1460,6 +1460,22 @@ A few guidelines for introducing and reviewing new RPC interfaces:
1460   - *Rationale*: JSON strings are Unicode strings, not byte strings, and
1461     RFC8259 requires JSON to be encoded as UTF-8.
1462 
1463+A few guidelines for modifying and reviewing existing RPC interfaces:
1464+
1465+- 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:
    l0rinc commented at 9:20 am on January 23, 2025:

    Long line, might want to break it up to simplify review suggestions:

    0- 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:
    tdb3 commented at 12:31 pm on January 23, 2025:
    Updated to data type and backward
  27. in doc/developer-notes.md:1466 in 0e7926e68e outdated 
    1459@@ -1460,6 +1460,22 @@ A few guidelines for introducing and reviewing new RPC interfaces:
1460   - *Rationale*: JSON strings are Unicode strings, not byte strings, and
1461     RFC8259 requires JSON to be encoded as UTF-8.
1462 
1463+A few guidelines for modifying and reviewing existing RPC interfaces:
1464+
1465+- 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:
1466+    ```c++
    l0rinc commented at 9:22 am on January 23, 2025:
    Would it make sense to refer to the code instead, to avoid duplication and maintenance burden?
    tdb3 commented at 12:32 pm on January 23, 2025:

    Thanks. Replaced with a permalink for it

    https://github.com/bitcoin/bitcoin/blob/94f0adcc31d2d263a03e46192922e3a7672e39d9/src/rpc/blockchain.cpp#L1316-L1323

  28. in doc/developer-notes.md:1477 in 0e7926e68e outdated 
    1472+            }
1473+            }
1474+        ),
1475+    ```
1476+
1477+  - *Rationale*: Changes in RPC JSON structure can break downstream application compatibility. Implementation of `deprecatedrpc` provides a grace-period for downstream applications to migrate. Release notes provide notification to downstream users.
    l0rinc commented at 9:24 am on January 23, 2025: 
    0  - *Rationale*: Changes in RPC JSON structure can break downstream application compatibility. Implementation of `deprecatedrpc` provides a grace period for downstream applications to migrate. Release notes provide notification to downstream users.
    tdb3 commented at 12:32 pm on January 23, 2025:
    Implemented
  29. l0rinc changes_requested
  30. l0rinc commented at 9:31 am on January 23, 2025: contributor
    No opinion about the content, reviewed only grammatically
  31. l0rinc changes_requested
  32. l0rinc commented at 9:31 am on January 23, 2025: contributor
    No opinion about the content, reviewed only grammatically
  33. doc: add guidance for RPC to developer notes 
    Adds deprecatedrpc guidance statement to the
RPC interface guidelines section.
    40ac07ef45
  34. tdb3 force-pushed on Jan 23, 2025
  35. l0rinc commented at 12:39 pm on January 23, 2025: contributor
    utACK 40ac07ef45f6de0ac6a13e34797387a6ee140de9
  36. DrahtBot requested review from epiccurious on Jan 23, 2025
  37. DrahtBot requested review from stickies-v on Jan 23, 2025


tdb3 DrahtBot jonatack epiccurious maflcko stickies-v l0rinc


laanwj stickies-v epiccurious

Labels
Docs

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-02-23 00:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me