doc: Add external interface consistency guarantees #14592
Conversation
|
Requested by @sdaftuar in #14193 (comment) and #14278 |
Reviewers, this pull request conflicts with the following ones:
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. |
ryanofsky
left a comment
There was a problem hiding this comment.
utACK faed8d54e4ab10406417e4149aa0620e18b2308b. It's good to add this documentation. I think it could be improved some more and I left some suggestions, but it's already helpful in its current form.
doc/JSON-RPC-interface.md
Outdated
There was a problem hiding this comment.
Should this say "mempool state returned via non-wallet RPCs" instead of "mempool state returned via an RPCs" since "wallet may not be up-to-date with the current state of the mempool"?
This might be clearer if it started off saying there are different types of RPCs (wallet and non-wallet RPCs), and then talked about each type of RPC in its own paragraph.
There was a problem hiding this comment.
I wouldn't call it different types of RPCs, but rather each rpc is part of one module. Modules could be:
- net
- chainstate
- mempool
- wallet
- ...
doc/JSON-RPC-interface.md
Outdated
There was a problem hiding this comment.
Can you give an example? Is this saying there that after a P2P response showing a certain state, there could be an RPC response returning an earlier state? Or is this just talking about incoming P2P messages that haven't been fully processed yet?
There was a problem hiding this comment.
Only talking about the processing delay/buffer.
Maybe I should just remove this sentence?
doc/JSON-RPC-interface.md
Outdated
There was a problem hiding this comment.
Would add a paragraph break before "The effects of all blocks..." because this is now changing topic to wallet RPCs.
Also maybe begin the paragraph with a sentence like "Wallet RPCs will return the latest chain state consistent with prior non-wallet RPCs, but not necessarily the latest mempool state" to give the next sentences some context.
doc/JSON-RPC-interface.md
Outdated
There was a problem hiding this comment.
What is a module? Could this be changed to say what a module is, or give examples or modules, or just avoid the term?
There was a problem hiding this comment.
Removed the mention of "modules"
doc/JSON-RPC-interface.md
Outdated
There was a problem hiding this comment.
Could this say "chain state" instead of "chainstate" to sound less jargonny and be clear this isn't referring to the chainstate folder?
doc/JSON-RPC-interface.md
Outdated
There was a problem hiding this comment.
Is this just talking about the wallet? Or are there other modules? This paragraph is vague and confusing to me, and I think it might be clearer if it just talked about RPC methods instead of modules.
There was a problem hiding this comment.
Removed the mention of "modules"
69ad521 to
47bb44f
Compare
ryanofsky
left a comment
There was a problem hiding this comment.
utACK 47bb44fb7c43ddf293c5398ad0d65303eda135ca
doc/JSON-RPC-interface.md
Outdated
There was a problem hiding this comment.
sent by this node, or sent by other nodes?
doc/JSON-RPC-interface.md
Outdated
There was a problem hiding this comment.
Maybe change "mempool state" with "mempool state returned via an RPC" to be consistent with paragraph above, and keep this grounded in RPCs, instead of the mempool abstractly.
doc/JSON-RPC-interface.md
Outdated
There was a problem hiding this comment.
Maybe I'm misunderstanding, but I think it's confusing to keep mentioning the mempool in this paragraph, when mempool consistency isn't explained until the next paragraph. Maybe drop "but not necessarily the latest mempool state" in the first sentence, and replace "mempool transactions" with "unconfirmed transactions" in this sentence.
ryanofsky
left a comment
There was a problem hiding this comment.
utACK cb97f99f6a38711af8a0a1e90400cd36774dfc3f. This looks good, and since it's documentation-only, maybe it could be merged, though it would be nice to have someone else take a look.
I keep suggesting new things every time I read this, so definitely feel free to ignore my suggestions.
doc/JSON-RPC-interface.md
Outdated
There was a problem hiding this comment.
Maybe say "reflect the mempool" rather than "depend on the mempool" to be more concrete.
cb97f99 to
fa77aaa
Compare
|
utACK fa77aaa appveyor failure seems BS |
fa77aaa doc: Add external interface consistency guarantees (MarcoFalke) Pull request description: An attempt to clarify our consistency guarantees for developers and users Tree-SHA512: 8bad6a2bcfd85f0aeeecf3b090332f64c778c69a838a519d11ea83f2cb51929b9f43a7e9b2469567f305a1277206cafe8e65041f1a002dadbe69259d6a0adc18
Summary: This adds a new documentation page about the JSON-RPC Interface Backport of Core [[bitcoin/bitcoin#14592 | PR14592]] Test Plan: Check markdown formatting in a markdown viewer. Reviewers: O1 Bitcoin ABC, #bitcoin_abc, deadalnix Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D7765
fa77aaa doc: Add external interface consistency guarantees (MarcoFalke) Pull request description: An attempt to clarify our consistency guarantees for developers and users Tree-SHA512: 8bad6a2bcfd85f0aeeecf3b090332f64c778c69a838a519d11ea83f2cb51929b9f43a7e9b2469567f305a1277206cafe8e65041f1a002dadbe69259d6a0adc18
fa77aaa doc: Add external interface consistency guarantees (MarcoFalke) Pull request description: An attempt to clarify our consistency guarantees for developers and users Tree-SHA512: 8bad6a2bcfd85f0aeeecf3b090332f64c778c69a838a519d11ea83f2cb51929b9f43a7e9b2469567f305a1277206cafe8e65041f1a002dadbe69259d6a0adc18
fa77aaa doc: Add external interface consistency guarantees (MarcoFalke) Pull request description: An attempt to clarify our consistency guarantees for developers and users Tree-SHA512: 8bad6a2bcfd85f0aeeecf3b090332f64c778c69a838a519d11ea83f2cb51929b9f43a7e9b2469567f305a1277206cafe8e65041f1a002dadbe69259d6a0adc18
An attempt to clarify our consistency guarantees for developers and users