Skip to content

rpc: Fail to return undocumented or misdocumented JSON#23083

Merged
fanquake merged 2 commits intobitcoin:masterfrom
maflcko:2108-docRpc
Mar 28, 2022
Merged

rpc: Fail to return undocumented or misdocumented JSON#23083
fanquake merged 2 commits intobitcoin:masterfrom
maflcko:2108-docRpc

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Sep 24, 2021

This avoids documentation shortcomings such as the ones fixed in commit 56c4ac5, e7b6272, 138d55e, 577bd51, f8c84e0, 0ee9a00, 13f4185, or faecb2e

@DrahtBot
Copy link
Contributor

DrahtBot commented Dec 2, 2021

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

Conflicts

No conflicts as of last run.

@maflcko maflcko force-pushed the 2108-docRpc branch 3 times, most recently from 8fda5d4 to fa42fe3 Compare December 3, 2021 09:20
@DrahtBot DrahtBot mentioned this pull request Dec 6, 2021
maflcko pushed a commit to bitcoin-core/gui that referenced this pull request Dec 7, 2021
…yDescription

fa1571b doc: Add missing optional to MempoolEntryDescription (MarcoFalke)

Pull request description:

  Needed for bitcoin/bitcoin#23083.

  Can be reviewed with `--word-diff-regex=.`.

ACKs for top commit:
  josibake:
    ACK bitcoin/bitcoin@fa1571b
  shaavan:
    ACK fa1571b

Tree-SHA512: b4370003d2aeadce438778e15bd9a0d6a7fef4711acbe8471a50a9d72bbf74e1705fecbaae6f7eb367ece7c795a816c4b8b6583ed6c8f91b35621ca30fd95c18
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 7, 2021
…tion

fa1571b doc: Add missing optional to MempoolEntryDescription (MarcoFalke)

Pull request description:

  Needed for bitcoin#23083.

  Can be reviewed with `--word-diff-regex=.`.

ACKs for top commit:
  josibake:
    ACK bitcoin@fa1571b
  shaavan:
    ACK fa1571b

Tree-SHA512: b4370003d2aeadce438778e15bd9a0d6a7fef4711acbe8471a50a9d72bbf74e1705fecbaae6f7eb367ece7c795a816c4b8b6583ed6c8f91b35621ca30fd95c18
@maflcko maflcko force-pushed the 2108-docRpc branch 2 times, most recently from faf8829 to fa32c3f Compare December 8, 2021 12:29
RandyMcMillan pushed a commit to RandyMcMillan/mempool-tab that referenced this pull request Dec 23, 2021
…yDescription

7f06b94 doc: Add missing optional to MempoolEntryDescription (MarcoFalke)

Pull request description:

  Needed for bitcoin/bitcoin#23083.

  Can be reviewed with `--word-diff-regex=.`.

ACKs for top commit:
  josibake:
    ACK bitcoin/bitcoin@7f06b94
  shaavan:
    ACK 7f06b94

Tree-SHA512: b4370003d2aeadce438778e15bd9a0d6a7fef4711acbe8471a50a9d72bbf74e1705fecbaae6f7eb367ece7c795a816c4b8b6583ed6c8f91b35621ca30fd95c18
Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK

maflcko pushed a commit to bitcoin-core/gui that referenced this pull request Mar 22, 2022
…info RPC docs

facd5d9 doc: Fix getblockchaininfo/getdeploymentinfo RPC docs (MarcoFalke)

Pull request description:

  Also, fix whitespace to be `4` spaces. Can be reviewed with `--ignore-all-space --word-diff-regex=.`.

  Found by bitcoin/bitcoin#23083

ACKs for top commit:
  luke-jr:
    crACK facd5d9

Tree-SHA512: 113228a6b140009cecd9068fb634d352148670589f647350e41c02a35e0ca306b4a2d3f2588cd9ef14a2ab7d1f23d0d2f83b5ebb00b60f17a1d16a8d71386fd2
@maflcko
Copy link
Member Author

maflcko commented Mar 22, 2022

I opened this less than 6 months ago and this already found 7 bugs (see linked pulls and OP)

fanquake added a commit to bitcoin-core/gui that referenced this pull request Mar 24, 2022
…excluded

faf37c2 rpc: Exclude descriptor when address is excluded (MarcoFalke)

Pull request description:

  I don't think output descriptors should be used to describe redeem scripts and witness scripts.

  Fix this by excluding them when it doesn't make sense.

  This should only affect the `decodepsbt` RPC.

  Found by bitcoin/bitcoin#23083

ACKs for top commit:
  achow101:
    ACK faf37c2
  jonatack:
    ACK faf37c2

Tree-SHA512: ebd581ad639e70080e26028723fed287caa3fa4d7b836936645020d6cd9b7586585d7113b043442c444a9dc90c23b93efd7f8b8a7d6cf5db1e42137b67c497c3
@maflcko maflcko mentioned this pull request Mar 28, 2022
fanquake added a commit that referenced this pull request Mar 28, 2022
faac877 doc: Fix getpeerinfo doc (MarcoFalke)

Pull request description:

  * Replace `node` with `peer`
  * Remove unused `\n`
  * Mark optional fields optional. See commit 9344697, found by #23083

ACKs for top commit:
  fanquake:
    ACK faac877

Tree-SHA512: ae4d52a0dcf8e132d9084e632d65fa835b1e7d0ed5c3d45a360570414d1e20bc7fb6500ff9be94b784af1dec5badcd1304153b1a4a59a6c484a87d8afd88b8bd
Copy link
Member

@fanquake fanquake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK fc892c3 - tested that this catches issue, i.e #24691:

test_framework.authproxy.JSONRPCException: Internal bug detected: 'std::any_of(m_results.m_results.begin(), m_results.m_results.end(), [ret](const RPCResult& res) { return res.MatchesType(ret); })'
rpc/util.cpp:587 (HandleRequest)
You may report this issue here: https://github.com/bitcoin/bitcoin/issues
 (-1)

This has been sitting catching bugs, too-long, to remain un-merged.

{RPCResult::Type::NUM, "duration", "elapsed seconds since scan start"},
{RPCResult::Type::NUM, "progress", "scanning progress percentage [0.0, 1.0]"},
}},
}, /*skip_type_check=*/true},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can / will this be removed in future?

@fanquake fanquake merged commit a13946b into bitcoin:master Mar 28, 2022
@maflcko maflcko deleted the 2108-docRpc branch March 28, 2022 11:23
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 2, 2022
faf37c2 rpc: Exclude descriptor when address is excluded (MarcoFalke)

Pull request description:

  I don't think output descriptors should be used to describe redeem scripts and witness scripts.

  Fix this by excluding them when it doesn't make sense.

  This should only affect the `decodepsbt` RPC.

  Found by bitcoin#23083

ACKs for top commit:
  achow101:
    ACK faf37c2
  jonatack:
    ACK faf37c2

Tree-SHA512: ebd581ad639e70080e26028723fed287caa3fa4d7b836936645020d6cd9b7586585d7113b043442c444a9dc90c23b93efd7f8b8a7d6cf5db1e42137b67c497c3
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 3, 2022
faac877 doc: Fix getpeerinfo doc (MarcoFalke)

Pull request description:

  * Replace `node` with `peer`
  * Remove unused `\n`
  * Mark optional fields optional. See commit 9344697, found by bitcoin#23083

ACKs for top commit:
  fanquake:
    ACK faac877

Tree-SHA512: ae4d52a0dcf8e132d9084e632d65fa835b1e7d0ed5c3d45a360570414d1e20bc7fb6500ff9be94b784af1dec5badcd1304153b1a4a59a6c484a87d8afd88b8bd
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 3, 2022
…d JSON

fc892c3 rpc: Fail to return undocumented or misdocumented JSON (MarcoFalke)
f4bc4a7 rpc: Add m_skip_type_check to RPCResult (MarcoFalke)

Pull request description:

  This avoids documentation shortcomings such as the ones fixed in commit e7b6272, 138d55e, 577bd51, f8c84e0, 0ee9a00, 13f4185, or faecb2e

ACKs for top commit:
  fanquake:
    ACK fc892c3 - tested that this catches issue, i.e bitcoin#24691:

Tree-SHA512: 9d0d7e6291bfc6f67541a4ff746d374ad8751fefcff6d103d8621c0298b190ab1d209ce96cfc3a0d4a6a5460a9f9bb790eb96027b16e5ff91f2512e40c92ca84
domob1812 added a commit to domob1812/namecoin-core that referenced this pull request Apr 7, 2022
Fix some issues with RPC documentation for Namecoin RPCs (e.g. missing
"optional" declaration for some result fields).  Since upstream merged
bitcoin/bitcoin#23083, these errors cause
the RPC calls to fail otherwise.
@bitcoin bitcoin locked and limited conversation to collaborators Mar 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants