/bitcoin

Add simple light-client mode (RPC only) #10794

Open
wants to merge 10 commits into
from
+316 −28

Conversation

Reviewers

@ryanofsky ryanofsky

Assignees
No one assigned
Labels
P2P Validation
Projects

In progress in Client-Mode (SPV)

Milestone
No milestone
2 participants
@jonasschnelli @ryanofsky
@jonasschnelli
Member

jonasschnelli commented Jul 11, 2017 • edited

This adds a simple light client mode (RPC only, no wallet support).

With this PR, It is possible to disable auto-request-blocks by passing in -autorequestblocks=0.
In that mode, one can request out-of-band blocks by calling requestblock add ["<blockhash>", ...].
Those blocks will then be requested/downloaded and can be loaded via getblock (and they will also be passed through ZMQ).

This allows a very simple light-client mode ideally when you already have a validated peer in your trusted network.

This is also a reviewable step towards light client mode for the wallet (which will ultimately allow process separation o the wallet).

@jonasschnelli jonasschnelli added P2P Validation labels Jul 11, 2017

jonasschnelli referenced this pull request Jul 11, 2017

Closed

Introduce auxiliary block requests #9171

@jonasschnelli
Member

jonasschnelli commented Jul 11, 2017

Contains overhauled parts from #9483
@ryanofsky
Contributor

ryanofsky commented Jul 11, 2017

I'd like to help out here, and I've spent literally days reviewing previous iterations of this change (#9076 #9483 #9171), but I can't figure out if this is going anywhere and if providing more review now is a good use of time.

As I've mentioned previously, I don't think the auxiliary block download class design is great, because it duplicates functionality from the networking layer in a wholly different part of the code, and gives the wallet too much control & responsibility over low level p2p details in the case where it's used to let the wallet code prioritize which blocks to download. I've tried to describe what I think would be better alternatives in my previous review comments, like #9483 (comment).

@jonasschnelli, assuming you don't want to take my previous suggestions, and do want to stick with the current design, I think it would be helpful if you could reach out to some other reviewers and get some concept acks for your current approach. It might help to put together some kind of design document, or at least a detailed comment to the top of auxiliaryblockrequest.h that lays out how the class works and what your future plans for it are in as much detail as possible, so someone can understand how it's intended to be used without having to wade through all the PRs.

ryanofsky referenced this pull request Jul 11, 2017

Open

Complete hybrid full block SPV mode #9483

@jonasschnelli
Member

jonasschnelli commented Jul 11, 2017

@ryanofsky:
Thank you very much for the reviews in #9483. I implemented almost all of your suggesting and some of them where really great.
However, I think the current design of having a dedicated class (CauxiliaryBlockRequest) makes sense, because...

  • An out-of-band (auxiliary) block request in an object, you could have multiple in parallel (assume multiwallet, etc.), in future, we may want to serialise them to disk, etc. Therefore I think it should be capsulated in its own class.

  • I think the wallet needs control. For a light-client mode, the wallet is the one that tells the node what it needs. More or less, wallet tells node: "I need block A to E, give them to me as fast as possible", then node may call back "I have block A already ready (on disk), rest will follow soon", etc. Wallet then may say: "Okay, thats enough, you can stop at B already", etc.

  • Having the implementation in a designated file makes code overview simpler. It can result in having slightly more lines of code but it should worth it. Having another main.cpp for everything that is network related should be avoided now, early enough. I don't think we have a lot of duplicated code, or can you point out what would be more compact if we would implement it directly in net_processing.cpp?

  • Last but not least, it allows to have a patch-set the requires less rebasing. And this is an important one to me. Eventually, this will be something that "runs along" with the master branch until there has been reasonable amount of testing before it gets eventually merged.

fanquake deleted a comment from MIGUELWAXX Jul 11, 2017

@jonasschnelli
Member

jonasschnelli commented Jul 12, 2017

Followed yesterdays discussion we had in #bitcoin-core-dev. Removed the CAuxiliaryBlockRequests and added a std::map blocksToDownloadFirst.
Such manually added, priority block downloads will not trigger ActivateBestChain.

This PR now also adds a new signal BlockProcessed().

The scope of this PR is not to make the block download interface flexible for multiple "users" (like the validation and eventually the light-client wallet).
@jonasschnelli
Member

jonasschnelli commented Jul 20, 2017

fixed @ryanofsky points.
This does pass travis now. Thanks in advance for reviews...
@ryanofsky

ryanofsky reviewed Jul 20, 2017
View changes

Change seems pretty straightforward. Main thing missing is a test for the new RPC call.

It would be useful to get concept ACKs from @gmaxwell and @sipa to make sure concerns from the earlier design discussion (https://botbot.me/freenode/bitcoin-core-dev/msg/88437543/) are addressed now.

I don't have a very clear idea of how this functionality is going to be used, and what future plans for it are, so I personally wouldn't mind seeing a list of what the next steps & future prs might look like, or knowing some applications that could use the new option & RPC. But probably these things are clearer to others.

@jonasschnelli
Member

jonasschnelli commented Jul 20, 2017

Thanks @ryanofsky. Agree about required conceptual ACKs from other devs.
The long term goal was sketched in #9483, basically a light client mode for Bitcoin-Core that would allow node/wallet separation.

jonasschnelli added to In progress in Client-Mode (SPV) Jul 21, 2017

@ryanofsky

ryanofsky reviewed Jul 24, 2017
View changes

utACK 1794a11. Change looks great: code & test are straightforward and commit history is well thought out.

I still can't figure out if new RPC is actually supposed to be useful for something other than debugging/testing. Seems fine if it isn't, though, because it's a simple wrapper around the functions for downloading blocks of order, which obviously are useful for SPV.

src/net_processing.cpp
@@ -311,7 +314,11 @@ void FinalizeNode(NodeId nodeid, bool& fUpdateConnectionTime) {
// Requires cs_main.
// Returns a bool indicating whether we requested this block.
@ryanofsky

ryanofsky Jul 24, 2017

Contributor

In commit "Add priority block request queue"

Should update comment for new return value

src/net_processing.cpp
@@ -466,9 +478,18 @@ void FindNextBlocksToDownload(NodeId nodeid, unsigned int count, std::vector<con
// Make sure pindexBestKnownBlock is up to date, we'll need it.
ProcessBlockAvailability(nodeid);
+ if (!blocksToDownloadFirst.empty()) {
+ for (auto& kv : blocksToDownloadFirst) {
@ryanofsky

ryanofsky Jul 24, 2017

Contributor

In commit "Add priority block request queue"

Could be const auto

src/rpc/blockchain.cpp
@@ -640,6 +644,7 @@ UniValue getblockheader(const JSONRPCRequest& request)
"{\n"
" \"hash\" : \"hash\", (string) the block hash (same as provided)\n"
" \"confirmations\" : n, (numeric) The number of confirmations, or -1 if the block is not on the main chain\n"
+ " \"validated\" : n, (boolean) True if the block has been validated (for priority block requests)\n"
@ryanofsky

ryanofsky Jul 24, 2017

Contributor

In commit "Add requestblocks - a simple way to priorize block downloads"

Maybe drop part of comment in parentheses, seems to imply value will be true for priority block requests.

src/rpc/blockchain.cpp
+ {
+ if (request.params.size() < 2)
+ throw JSONRPCError(RPC_INVALID_PARAMETER, "Missing blocks array");
+ UniValue hash_Uarray = request.params[1].get_array();
@ryanofsky

ryanofsky Jul 24, 2017

Contributor

In commit "[RPC] Add requestblocks - a simple way to priorize block downloads"

Copy is redundant, get_array just returns reference to *this.

src/rpc/blockchain.cpp
+ throw JSONRPCError(RPC_INVALID_PARAMETER, "Missing blocks array");
+ UniValue hash_Uarray = request.params[1].get_array();
+ if (!hash_Uarray.isArray())
+ throw JSONRPCError(RPC_INVALID_PARAMETER, "Second parameter must be an array");
@ryanofsky

ryanofsky Jul 24, 2017

Contributor

In commit "[RPC] Add requestblocks - a simple way to priorize block downloads"

Can never happen, get_array call above would have thrown if the hash_Uarray / params[1] value was not an array

src/rpc/blockchain.cpp
+ BlockMap::iterator mi = mapBlockIndex.find(hash);
+ if (mi == mapBlockIndex.end())
+ throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block not found");
+ blocksToDownload.push_back((*mi).second);
@ryanofsky

ryanofsky Jul 24, 2017

Contributor

In commit "[RPC] Add requestblocks - a simple way to priorize block downloads"

Probably should write mi->second

@jonasschnelli
Member

jonasschnelli commented Jul 25, 2017

Overhauled and fixed @ryanofsky points (mostly nits), also removed the new (unused) signal.
@ryanofsky

ryanofsky reviewed Jul 25, 2017
View changes

utACK 9723672. Changes since last review were dropping a commit that added a BlockProcessed signal, changing order of other commits, changing brace styles, and making various suggested tweaks.

src/rpc/blockchain.cpp
+ return ret;
+ }
+ else if (request.params[0].get_str() == "add") {
+ if (request.params.size() < 2) {
@ryanofsky

ryanofsky Jul 25, 2017

Contributor

In commit "[RPC] Add requestblocks - a simple way to priorize block downloads"

Don't really need this check, because the get_array call below will trigger it's own error about the value not being an array. But if you prefer this error, you should change the check to request.params[1].isNull() to avoid making a distinction between a null and a missing value (#10281, #10783).

@jonasschnelli
Member

jonasschnelli commented Jul 26, 2017

Overhauled first commit to make sure we request blocks in order:
5927f7f#diff-eff7adeaec73a769788bb78858815c91R483
@jonasschnelli
Member

jonasschnelli commented Jul 27, 2017

  • Overhauled once again. Added the processing logic to ensure blocks are processed in order (makes it much simpler to process by "the other side").
  • Added a new main signal for the processing (reusing BlockChecked seems wrong).
  • Priority requests are now pushed through signal

This is now tested on my SPV branch and could be the first reviewable step towards light clients / wallet process separation.

jonasschnelli added some commits Jul 12, 2017

@jonasschnelli jonasschnelli Add priority block request queue
This commit was signed with a verified signature.
@jonasschnelli jonasschnelli Jonas Schnelli
GPG key ID: 1EB776BB03C7922D Learn about signing commits
70c07b0
@jonasschnelli jonasschnelli Don't ActivateBestChain when processing priority block request
This commit was signed with a verified signature.
@jonasschnelli jonasschnelli Jonas Schnelli
GPG key ID: 1EB776BB03C7922D Learn about signing commits
bc78a0c
@jonasschnelli jonasschnelli Add new ProcessPriorityRequest main signal
This commit was signed with a verified signature.
@jonasschnelli jonasschnelli Jonas Schnelli
GPG key ID: 1EB776BB03C7922D Learn about signing commits
a51731b
@jonasschnelli jonasschnelli Process priority requests
This commit was signed with a verified signature.
@jonasschnelli jonasschnelli Jonas Schnelli
GPG key ID: 1EB776BB03C7922D Learn about signing commits
8aed8fe
@jonasschnelli jonasschnelli [RPC] Add requestblocks - a simple way to priorize block downloads
This commit was signed with a verified signature.
@jonasschnelli jonasschnelli Jonas Schnelli
GPG key ID: 1EB776BB03C7922D Learn about signing commits
ed4d1ec
@jonasschnelli jonasschnelli Add fAutoRequestBlocks to disabled/enable the verification progress
This commit was signed with a verified signature.
@jonasschnelli jonasschnelli Jonas Schnelli
GPG key ID: 1EB776BB03C7922D Learn about signing commits
e685c26
@jonasschnelli jonasschnelli Optionally allow to skip the toFarAway check in AcceptBlock
This commit was signed with a verified signature.
@jonasschnelli jonasschnelli Jonas Schnelli
GPG key ID: 1EB776BB03C7922D Learn about signing commits
b960d29
@jonasschnelli jonasschnelli Add -autorequestblocks which then allows to run in non-validation mod… 
…e (pure light-client mode)
This commit was signed with a verified signature.
@jonasschnelli jonasschnelli Jonas Schnelli
GPG key ID: 1EB776BB03C7922D Learn about signing commits
2451b81
@jonasschnelli jonasschnelli [QA] add requestblocks test
This commit was signed with a verified signature.
@jonasschnelli jonasschnelli Jonas Schnelli
GPG key ID: 1EB776BB03C7922D Learn about signing commits
a949b1c
@jonasschnelli jonasschnelli Pass priority requests through ZMQ notifications
This commit was signed with a verified signature.
@jonasschnelli jonasschnelli Jonas Schnelli
GPG key ID: 1EB776BB03C7922D Learn about signing commits
f7223dd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment