Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

doc: Add external interface consistency guarantees #14592

Merged
merged 1 commit into from Nov 1, 2018

Conversation

Projects
None yet
4 participants
@MarcoFalke
Copy link
Member

commented Oct 28, 2018

An attempt to clarify our consistency guarantees for developers and users

@MarcoFalke MarcoFalke added the Docs label Oct 28, 2018

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

commented Oct 28, 2018

Requested by @sdaftuar in #14193 (comment) and #14278

@bitcoin bitcoin deleted a comment from WorkShop-Office Oct 28, 2018

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Oct 28, 2018

Reviewers, this pull request conflicts with the following ones:
  • #14458 (WIP: Add JSON-RPC interface documentation by promag)

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
Copy link
Contributor

left a 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

In practice that means:

* The mempool state returned via an RPC is consistent with itself and with the

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Oct 29, 2018

Contributor

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.

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Oct 29, 2018

Author Member

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
chainstate at the time of the call. Thus, the mempool state only encompasses
transactions that are considered mine-able by the node at the time of the
RPC. The mempool state may not reflect all effects of blocks and transactions
that were sent on the P2P interface, but it reflects all effects of mempool

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Oct 29, 2018

Contributor

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?

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Oct 29, 2018

Author Member

Only talking about the processing delay/buffer.

Maybe I should just remove this sentence?

doc/JSON-RPC-interface.md Outdated
that were sent on the P2P interface, but it reflects all effects of mempool
and chainstate related RPCs that returned prior to this call.
* The wallet state returned via an RPC is consistent with itself and with the
chainstate at the time of the call. The effects of all blocks (and

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Oct 29, 2018

Contributor

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

## RPC consistency guarantees

State of modules that can be queried via RPCs is guaranteed to be at least

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Oct 29, 2018

Contributor

What is a module? Could this be changed to say what a module is, or give examples or modules, or just avoid the term?

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Oct 29, 2018

Author Member

Removed the mention of "modules"

doc/JSON-RPC-interface.md Outdated
## RPC consistency guarantees

State of modules that can be queried via RPCs is guaranteed to be at least
up-to-date with the chainstate immediately prior to the call's execution.

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Oct 29, 2018

Contributor

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

State of modules that can be queried via RPCs is guaranteed to be at least
up-to-date with the chainstate immediately prior to the call's execution.
However, the state of modules that depend on the mempool may not be up-to-date

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Oct 29, 2018

Contributor

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.

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Oct 29, 2018

Author Member

Removed the mention of "modules"

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1810-docCon branch 2 times, most recently Oct 29, 2018

@ryanofsky
Copy link
Contributor

left a comment

utACK 47bb44fb7c43ddf293c5398ad0d65303eda135ca

doc/JSON-RPC-interface.md Outdated
RPC.

The mempool state may not reflect all effects of blocks and transactions
that were sent on the P2P interface, but it reflects all effects of mempool

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Oct 29, 2018

Contributor

sent by this node, or sent by other nodes?

doc/JSON-RPC-interface.md Outdated
transactions that are considered mine-able by the node at the time of the
RPC.

The mempool state may not reflect all effects of blocks and transactions

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Oct 29, 2018

Contributor

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
non-wallet RPCs, but not necessarily the latest mempool state. The effects of
all blocks (and transactions in blocks) at the time of the call is reflected
in the state of all wallet transactions. For example, if a block contains
transactions that conflicted with mempool transactions, the wallet would

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Oct 29, 2018

Contributor

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
Copy link
Contributor

left a 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

State that can be queried via RPCs is guaranteed to be at least up-to-date with
the chain state immediately prior to the call's execution. However, the state
returned by RPCs that depend on the mempool may not be up-to-date with the

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Oct 30, 2018

Contributor

Maybe say "reflect the mempool" rather than "depend on the mempool" to be more concrete.

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1810-docCon branch to fa77aaa Oct 30, 2018

@laanwj

This comment has been minimized.

Copy link
Member

commented Nov 1, 2018

utACK fa77aaa

appveyor failure seems BS

@laanwj laanwj merged commit fa77aaa into bitcoin:master Nov 1, 2018

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Nov 1, 2018

Merge #14592: doc: Add external interface consistency guarantees
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

@MarcoFalke MarcoFalke deleted the MarcoFalke:Mf1810-docCon branch Nov 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.