jonasschnelli commented at 3:01 pm on July 11, 2017: contributor

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 added the label P2P on Jul 11, 2017

jonasschnelli added the label Validation on Jul 11, 2017

jonasschnelli commented at 3:02 pm on July 11, 2017: contributor

Contains overhauled parts from #9483

ryanofsky commented at 5:03 pm on July 11, 2017: member

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.

jonasschnelli commented at 6:36 pm on July 11, 2017: contributor

@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 on Jul 11, 2017

jonasschnelli force-pushed on Jul 12, 2017

jonasschnelli commented at 3:47 pm on July 12, 2017: contributor

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 force-pushed on Jul 12, 2017

jonasschnelli force-pushed on Jul 17, 2017

jonasschnelli force-pushed on Jul 18, 2017

jonasschnelli force-pushed on Jul 19, 2017

jonasschnelli force-pushed on Jul 20, 2017

jonasschnelli commented at 6:11 pm on July 20, 2017: contributor

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

ryanofsky commented at 6:59 pm on July 20, 2017: member

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 commented at 7:58 pm on July 20, 2017: contributor

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 force-pushed on Jul 21, 2017

jonasschnelli added this to the "In progress" column in a project

in src/net_processing.cpp:315 in b02b237a00 outdated

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

ryanofsky commented at 2:16 pm on July 24, 2017:

In commit “Add priority block request queue”

Should update comment for new return value

in src/net_processing.cpp:482 in b02b237a00 outdated

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

ryanofsky commented at 2:21 pm on July 24, 2017:

In commit “Add priority block request queue”

Could be const auto

in src/rpc/blockchain.cpp:647 in 701014215f outdated

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

ryanofsky commented at 2:32 pm on July 24, 2017:

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.

in src/rpc/blockchain.cpp:1578 in 701014215f outdated

1573+    }
1574+    else if (request.params[0].get_str() == "add")
1575+    {
1576+        if (request.params.size() < 2)
1577+            throw JSONRPCError(RPC_INVALID_PARAMETER, "Missing blocks array");
1578+        UniValue hash_Uarray = request.params[1].get_array();

ryanofsky commented at 2:36 pm on July 24, 2017:

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

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

in src/rpc/blockchain.cpp:1580 in 701014215f outdated

1575+    {
1576+        if (request.params.size() < 2)
1577+            throw JSONRPCError(RPC_INVALID_PARAMETER, "Missing blocks array");
1578+        UniValue hash_Uarray = request.params[1].get_array();
1579+        if (!hash_Uarray.isArray())
1580+            throw JSONRPCError(RPC_INVALID_PARAMETER, "Second parameter must be an array");

ryanofsky commented at 2:37 pm on July 24, 2017:

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

in src/rpc/blockchain.cpp:1591 in 701014215f outdated

1586+            {
1587+                uint256 hash(uint256S(strHashU.get_str()));
1588+                BlockMap::iterator mi = mapBlockIndex.find(hash);
1589+                if (mi == mapBlockIndex.end())
1590+                    throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block not found");
1591+                blocksToDownload.push_back((*mi).second);

ryanofsky commented at 2:38 pm on July 24, 2017:

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

Probably should write mi->second

ryanofsky commented at 3:06 pm on July 24, 2017: member

utACK 1794a11037150b0d2c6ba2eb9f06e310f51f7c1c. 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.

jonasschnelli force-pushed on Jul 25, 2017

jonasschnelli commented at 8:50 am on July 25, 2017: contributor

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

in src/rpc/blockchain.cpp:1625 in d3e64f6005 outdated

1568+            ret.push_back(kv.first.ToString());
1569+        }
1570+        return ret;
1571+    }
1572+    else if (request.params[0].get_str() == "add") {
1573+        if (request.params.size() < 2) {

ryanofsky commented at 3:09 pm on July 25, 2017:

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

ryanofsky commented at 3:19 pm on July 25, 2017: member

utACK 9723672ced8b48eeee4847e6c0e0ed39833c7eaf. 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.

jonasschnelli force-pushed on Jul 26, 2017

jonasschnelli commented at 2:04 pm on July 26, 2017: contributor

Overhauled first commit to make sure we request blocks in order: https://github.com/bitcoin/bitcoin/pull/10794/commits/5927f7f1d6c7eb1bc7fb7a39dfda277ad5e4474c#diff-eff7adeaec73a769788bb78858815c91R483

jonasschnelli force-pushed on Jul 27, 2017

jonasschnelli commented at 8:33 am on July 27, 2017: contributor

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 force-pushed on Jul 27, 2017

jtimon commented at 9:24 pm on August 31, 2017: contributor

Needs rebase

nopara73 commented at 6:06 pm on October 3, 2017: none

@ryanofsky Reflecting to your usefullness concern:
I wrote a full block downloading SPV wallet (HiddenWallet), the main reason is, because without such architecture private transaction retrieval is hard, if not impossible.
When some form of light functionality gets into Core I would personally consider porting HiddenWallet to on top of Core, since I cannot compete with Core’s speed and stability.

ryanofsky commented at 6:48 pm on October 3, 2017: member

That’s really interesting. @nopara73, if I understand you correctly, you are saying that if the requestblocks JSON-RPC API from this PR is added to bitcoin core, then HiddenWallet would be able to call it to download blocks instead of depending on NBitcoin?

nopara73 commented at 7:06 pm on October 3, 2017: none

@ryanofsky

you are saying that if the requestblocks JSON-RPC API from this PR is added to bitcoin core, then HiddenWallet would be able to call it to download blocks instead of depending on NBitcoin?

Yes.

jonasschnelli force-pushed on Nov 22, 2017

jonasschnelli commented at 10:23 pm on November 22, 2017: contributor

Rebased

in src/net_processing.cpp:143 in 9ee22bf4a0 outdated

138@@ -136,6 +139,13 @@ namespace {
139     MapRelay mapRelay;
140     /** Expiration-time ordered list of (expire time, relay map entry) pairs, protected by cs_main). */
141     std::deque<std::pair<int64_t, MapRelay::iterator>> vRelayExpiration;
142+
143+    struct PriorityBlockRequest {

promag commented at 9:37 am on November 23, 2017:

Nit, should be

0struct PriorityBlockRequest
1{

promag commented at 10:30 am on November 23, 2017:

Same for other structs.

in src/net_processing.cpp:327 in 9ee22bf4a0 outdated

324-bool MarkBlockAsReceived(const uint256& hash) {
325+struct MarkBlockAsReceivedResult {
326+    bool fRequested;
327+    bool fPriorityRequest;
328+ };
329+const MarkBlockAsReceivedResult MarkBlockAsReceived(const uint256& hash) {

promag commented at 9:38 am on November 23, 2017:

Nit, remove const from return?

in src/rpc/blockchain.cpp:1599 in 9ee22bf4a0 outdated

1591@@ -1587,6 +1592,60 @@ UniValue savemempool(const JSONRPCRequest& request)
1592     return NullUniValue;
1593 }
1594 
1595+UniValue requestblocks(const JSONRPCRequest& request)
1596+{
1597+    if (request.fHelp || request.params.size() < 1 || request.params.size() > 3)
1598+        throw std::runtime_error(
1599+            "requestblocks (add|flush|status) ([\"hash_0\", \"hash_1\", ...])\n"

promag commented at 9:39 am on November 23, 2017:

Nit, missing spaces after ( and before ).

in src/rpc/blockchain.cpp:1657 in 9ee22bf4a0 outdated

1614+            );
1615+
1616+    if (request.params[0].get_str() == "flush") {
1617+        return UniValue(FlushPriorityDownloads());
1618+    }
1619+    else if (request.params[0].get_str() == "status") {

promag commented at 9:42 am on November 23, 2017:

Nit, remove else’s?

jonasschnelli commented at 9:03 pm on November 23, 2017:

Why?

in src/rpc/blockchain.cpp:1632 in 9ee22bf4a0 outdated

1627+        }
1628+        std::vector<const CBlockIndex*> blocksToDownload;
1629+        {
1630+            LOCK(cs_main); //mapBlockIndex
1631+            for (const UniValue& strHashU : request.params[1].get_array().getValues())
1632+            {

promag commented at 9:43 am on November 23, 2017:

Nit, move up.

in test/functional/requestblocks.py:46 in 9ee22bf4a0 outdated

41+        assert_raises_rpc_error(-1, "Block not found on disk", self.nodes[1].getblock, node0bbhash)
42+
43+        # request best block (auxiliary)
44+        self.nodes[1].requestblocks("add", [node0bbhash])
45+        timeout = 20
46+        while timeout > 0:

promag commented at 9:47 am on November 23, 2017:

use wait_until?

promag commented at 9:50 am on November 23, 2017:

Another option would be to use zmq notification, another PR maybe?

in test/functional/requestblocks.py:55 in 9ee22bf4a0 outdated

50+            timeout-=1
51+        assert(timeout>0)
52+
53+        # block must now be available
54+        block = self.nodes[1].getblock(node0bbhash, True)
55+        assert(block['hash'] == node0bbhash)

promag commented at 9:48 am on November 23, 2017:

assert_equal?

in test/functional/requestblocks.py:56 in 9ee22bf4a0 outdated

51+        assert(timeout>0)
52+
53+        # block must now be available
54+        block = self.nodes[1].getblock(node0bbhash, True)
55+        assert(block['hash'] == node0bbhash)
56+        assert(block['validated'] == False)

promag commented at 9:48 am on November 23, 2017:

assert_equal?

in test/functional/requestblocks.py:39 in 9ee22bf4a0 outdated

34+
35+        node0bbhash = self.nodes[0].getbestblockhash()
36+        # best block should not be validated, header must be available
37+        bh = self.nodes[1].getblockheader(node0bbhash, True)
38+
39+        assert(bh['validated'] == False)

promag commented at 9:50 am on November 23, 2017:

assert_equal?

in test/functional/requestblocks.py:20 in 9ee22bf4a0 outdated

15+
16+    def run_test(self):
17+        self.nodes[0].generate(50)
18+        timeout = 20
19+        ctps = self.nodes[1].getchaintips()
20+        while timeout > 0:

promag commented at 9:51 am on November 23, 2017:

Use wait_until?

in test/functional/requestblocks.py:62 in 9ee22bf4a0 outdated

57+
58+        #prevblock must not be available
59+        assert_raises_rpc_error(-1, "Block not found on disk", self.nodes[1].getblock, block['previousblockhash'])
60+
61+if __name__ == '__main__':
62+    RequestBlockRequestTest ().main ()

promag commented at 9:51 am on November 23, 2017:

Nit, remove spaces?

in src/rpc/blockchain.cpp:1647 in 9ee22bf4a0 outdated

1604+            "                                        flush = flush the queue (blocks in-flight will still be downloaded)\n"
1605+            "                                        status = get info about the queue\n"
1606+            "2. blockhashes       (array, optional) the hashes of the blocks to download\n"
1607+            "\nResult:\n"
1608+            "   add: <null>\n"
1609+            "   flush: <true|false> (if the the queue wasn't empty)\n"

promag commented at 9:53 am on November 23, 2017:

Missing test for flush.

promag commented at 10:15 am on November 23, 2017:

What is the purpose of the return value?

jonasschnelli commented at 9:01 pm on November 23, 2017:

You can determine if you have flushed something or not.

in src/rpc/blockchain.cpp:1664 in 9ee22bf4a0 outdated

1621+        ret.push_back(Pair("count", (uint64_t)CountPriorityDownloads()));
1622+        return ret;
1623+    }
1624+    else if (request.params[0].get_str() == "add") {
1625+        if (request.params.size() < 2) {
1626+            throw JSONRPCError(RPC_INVALID_PARAMETER, "Missing blocks array");

promag commented at 9:57 am on November 23, 2017:

Missing test for this error.

in src/rpc/blockchain.cpp:1635 in 9ee22bf4a0 outdated

1630+            LOCK(cs_main); //mapBlockIndex
1631+            for (const UniValue& strHashU : request.params[1].get_array().getValues())
1632+            {
1633+                uint256 hash(uint256S(strHashU.get_str()));
1634+                BlockMap::iterator mi = mapBlockIndex.find(hash);
1635+                if (mi == mapBlockIndex.end())

promag commented at 9:58 am on November 23, 2017:

Nit, add { }

in src/rpc/blockchain.cpp:1634 in 9ee22bf4a0 outdated

1629+        {
1630+            LOCK(cs_main); //mapBlockIndex
1631+            for (const UniValue& strHashU : request.params[1].get_array().getValues())
1632+            {
1633+                uint256 hash(uint256S(strHashU.get_str()));
1634+                BlockMap::iterator mi = mapBlockIndex.find(hash);

promag commented at 9:58 am on November 23, 2017:

BlockMap::const_iterator?

in src/rpc/blockchain.cpp:1597 in 9ee22bf4a0 outdated

1591@@ -1587,6 +1592,60 @@ UniValue savemempool(const JSONRPCRequest& request)
1592     return NullUniValue;
1593 }
1594 
1595+UniValue requestblocks(const JSONRPCRequest& request)
1596+{
1597+    if (request.fHelp || request.params.size() < 1 || request.params.size() > 3)

promag commented at 10:02 am on November 23, 2017:

Should be > 2?

in src/rpc/blockchain.cpp:687 in 9ee22bf4a0 outdated

648@@ -645,6 +649,7 @@ UniValue getblockheader(const JSONRPCRequest& request)
649             "{\n"
650             "  \"hash\" : \"hash\",     (string) the block hash (same as provided)\n"
651             "  \"confirmations\" : n,   (numeric) The number of confirmations, or -1 if the block is not on the main chain\n"
652+            "  \"validated\" : n,       (boolean) True if the block has been validated\n"

promag commented at 10:04 am on November 23, 2017:

Should update getblockheader test? https://github.com/bitcoin/bitcoin/blob/84fa64598e8b7d6901c546e938fb0eeda625a8e8/test/functional/blockchain.py#L173-L198

jonasschnelli commented at 9:15 pm on November 23, 2017:

done

in src/net_processing.h:91 in 9ee22bf4a0 outdated

82@@ -81,4 +83,18 @@ bool GetNodeStateStats(NodeId nodeid, CNodeStateStats &stats);
83 /** Increase a node's misbehavior score. */
84 void Misbehaving(NodeId nodeid, int howmuch);
85 
86+/**
87+ * Prioritize a block for downloading
88+ * Blocks requested with priority will be downloaded and processed first
89+ * Downloaded blocks will not trigger ActivateBestChain
90+ */
91+void AddPriorityDownload(std::vector<const CBlockIndex*>& blocksToDownload);

promag commented at 10:06 am on November 23, 2017:

Arg reference should be const:

0const std::vector<const CBlockIndex*>& blocks_to_download

in src/net_processing.cpp:3757 in 9ee22bf4a0 outdated

3752+    blocksToDownloadFirst.clear();
3753+    return !ret;
3754+}
3755+
3756+size_t CountPriorityDownloads() {
3757+    // return a copy

promag commented at 10:07 am on November 23, 2017:

Copy of?

in src/net_processing.cpp:3810 in 9ee22bf4a0 outdated

3730+                throw std::runtime_error(std::string(__func__) + "Can't read block from disk");
3731+            }
3732+            currentBlock = std::make_shared<const CBlock>(loadBlock);
3733+        }
3734+        else {
3735+            break;

promag commented at 10:11 am on November 23, 2017:

Add comment explaining this case?

in src/net_processing.cpp:3785 in 9ee22bf4a0 outdated

3706+        blocksToDownloadFirst.push_back({pindex, false});
3707+    }
3708+}
3709+
3710+void ProcessPriorityRequests(const std::shared_ptr<CBlock> blockRef) {
3711+    LOCK(cs_main);

promag commented at 10:11 am on November 23, 2017:

cs_main is locked throughout. It is enough to lock when dequeuing right? If dequeuing fails then return.

in src/net_processing.cpp:3740 in 9ee22bf4a0 outdated

3735+            break;
3736+        }
3737+
3738+        // allow processing through signal
3739+        GetMainSignals().ProcessPriorityRequest(currentBlock, r.pindex);
3740+        LogPrint(BCLog::NET, "process priority block request (%s) height=%d\n", r.pindex->GetBlockHash().ToString(), r.pindex->nHeight);

promag commented at 10:12 am on November 23, 2017:

Move log before signal?

jonasschnelli commented at 8:49 pm on November 23, 2017:

I think it’s okay after, but should say “processed” instead of “process”.

in src/net_processing.cpp:3818 in 9ee22bf4a0 outdated

3738+        // allow processing through signal
3739+        GetMainSignals().ProcessPriorityRequest(currentBlock, r.pindex);
3740+        LogPrint(BCLog::NET, "process priority block request (%s) height=%d\n", r.pindex->GetBlockHash().ToString(), r.pindex->nHeight);
3741+
3742+        // remove processed block from queue
3743+        it = blocksToDownloadFirst.erase(std::remove_if(blocksToDownloadFirst.begin(), blocksToDownloadFirst.end(), [&r](const PriorityBlockRequest &rB) {

promag commented at 10:14 am on November 23, 2017:

Is removing duplicates here really necessary? Blocks with BLOCK_HAVE_DATA will be skipped.

jonasschnelli commented at 8:51 pm on November 23, 2017:

With the current implementation, blocksToDownloadFirst can have multiple of the same block. I’d say it should remain here…

in src/net_processing.h:95 in 9ee22bf4a0 outdated

90+ */
91+void AddPriorityDownload(std::vector<const CBlockIndex*>& blocksToDownload);
92+void ProcessPriorityRequests(const std::shared_ptr<CBlock> block);
93+bool FlushPriorityDownloads();
94+size_t CountPriorityDownloads();
95+void ProcessPriorityRequests(const std::shared_ptr<CBlock> block);

promag commented at 10:16 am on November 23, 2017:

Remove, duplicate declaration.

in src/net_processing.cpp:504 in 9ee22bf4a0 outdated

497@@ -473,9 +498,26 @@ void FindNextBlocksToDownload(NodeId nodeid, unsigned int count, std::vector<con
498     // Make sure pindexBestKnownBlock is up to date, we'll need it.
499     ProcessBlockAvailability(nodeid);
500 
501+    if (!blocksToDownloadFirst.empty()) {
502+        for (const PriorityBlockRequest &r: blocksToDownloadFirst) {
503+            if (r.downloaded) continue;
504+            if (r.pindex && state->pindexBestKnownBlock != NULL && state->pindexBestKnownBlock->nHeight >= r.pindex->nHeight && !mapBlocksInFlight.count(r.pindex->GetBlockHash())) {

promag commented at 10:26 am on November 23, 2017:

Split in multiple tests. Suggestion for loop body:

0if (!r.pindex) continue;
1if (state->pindexBestKnownBlock == nullptr) continue;
2if (state->pindexBestKnownBlock->nHeight < r.pindex->nHeight) continue;
3if (mapBlocksInFlight.count(r.pindex->GetBlockHash())) continue;
4
5vBlocks.push_back(r.pindex);
6
7if (vBlocks.size() == count) break;

IMHO more readable and more easier to add comments.

promag commented at 10:28 am on November 23, 2017:

Otherwise, replace NULL with nullptr and use break instead of return true.

promag commented at 10:29 am on November 23, 2017: member

LGTM, will play a bit, meanwhile some comments below.

Test that flush empties the queue:

disconnect node
requestblocks add [“valid hash”]
requestblocks flush
requestblocks status
assert count equal 0

Test that no block is enqueued if one is unknown:

requestblocks add [“valid hash”, “invalid hash”]
assert the above error
requestblocks status
assert count equal 0

in src/rpc/blockchain.cpp:1655 in 9ee22bf4a0 outdated

1612+            + HelpExampleCli("requestblocks", "add, \"'[\"<blockhash>\"]'\"")
1613+            + HelpExampleRpc("requestblocks", "add, \"'[\"<blockhash>\"]'\"")
1614+            );
1615+
1616+    if (request.params[0].get_str() == "flush") {
1617+        return UniValue(FlushPriorityDownloads());

promag commented at 11:24 am on November 23, 2017:

Assert request.params[1] is null. Same for status.

nopara73 commented at 7:08 pm on November 23, 2017: none

Simple RPC App Example

Just to be sure I understand correctly this PR and the application I am considering will work as I expect to work:

How would the wallet behave? Best effort or disabled?

So let’s say is it possible to write a software, that uses Bitcoin Core’s wallet with autorequestblocks=0 in the following way:

Sync up the header.
Create a wallet, save the creation time (or block height).
Subscribe for new header notifications.
Every time a new header comes call requestblock add blockhash

So the wallet, from the information it has available executes the rpc commands properly based on best effort, right? And is not supposed to fail.

This gives me the possibility to implement leapfrogging for example. (Enable the user to say: hey, I didn’t do or wait for any transaction in the last 3 months and I have no intention to sync it up, it’s ok if I get the blocks from now on.)

Other Concerns

Since this mode cannot have up to date utxo set, how do you broadcast transactions? How do you make sure it’s valid?
How do you accept transactions to your mempool without making sure it spends an utxo? Are you also propagating them?
What would getblockcount return?

jonasschnelli commented at 8:05 pm on November 23, 2017: contributor

@nopara73

This PR does not touch the wallet (that will come later, would be to large of a PR).
This PR does allow to disable the auto-download (sync)
This PR does allow to download random blocks (specified by block-hashes) and have them header-only-validated, … so you can parse the blocks content

Since this mode cannot have up to date utxo set, how do you broadcast transactions? How do you make sure it’s valid?

This is not done on the PR. But there is a commit available in #9483 https://github.com/bitcoin/bitcoin/pull/9483/commits/7ca1a8738a878078c0d1546f7743a05ee474dd1b that does allow broadcasting transactions without an utxo set.

How do you accept transactions to your mempool without making sure it spends an utxo? Are you also propagating them?

This is impossible in light client mode. Mempool transactions (zero confirmation txns) are pretty unsafe in light client mode.

What would getblockcount return?

The same (probably 0 in autorequestblocks=0 mode). Use getchaintips() to check your headers (not super efficient).

jonasschnelli force-pushed on Nov 23, 2017

nopara73 commented at 10:04 pm on November 23, 2017: none

@jonasschnelli I’d like to make the last couple of things clear before I compile and test it, just to know what to expect.

1

there is a commit available in #9483 7ca1a87 that does allow broadcasting transactions without an utxo set.

Meaning, sendrawtransaction will always return null?

2

This PR does not touch the wallet (that will come later, would be to large of a PR).

Meaning, (1) disables the wallet, (2) makes the wallet rpcs return sometimes incorrect results, (3) you don’t know what the rpcs would return?

jonasschnelli commented at 5:32 am on November 27, 2017: contributor

Meaning, sendrawtransaction will always return null?

No. It will return the txid (as expected). But not in this PR (that would be with something like https://github.com/bitcoin/bitcoin/commit/7ca1a8738a878078c0d1546f7743a05ee474dd1b).

Meaning, (1) disables the wallet, (2) makes the wallet rpcs return sometimes incorrect results, (3) you don’t know what the rpcs would return?

The wallet runs the same way as in master. All wallet commands do work exactly the same way as the do in standard IBD mode. Blocks processed with the new requestblocks will be ignored by the wallet (otherwise this PR would be too large).

The scope of this PR is to add the necessary network processing infrastructure to later add an SPV mode to the wallet.

ryanofsky commented at 5:46 pm on January 9, 2018: member

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.

Agree about required conceptual ACKs from other devs.

Last I looked code was in good shape, but I don’t think there was ever more design feedback on this. Maybe it should be brought up at an IRC meeting?

nopara73 commented at 8:10 am on February 20, 2018: none

This will be especially useful when combined with BIP158 (Compact Block Filters for Light Clients.) https://github.com/bitcoin/bips/blob/master/bip-0158.mediawiki

I imagine, based on this PR someone will eventually make a Bitcoin Core light client after Core starts to serve filters. At least that’ll be a low hanging fruit.

Sjors commented at 2:50 pm on February 20, 2018: member

Though I’m not sure what it would do on its own, perhaps the PriorityBlockRequest could be a separate PR?

Same for requestblocks RPC; could that be useful in pruned nodes to retrieve a pruned block?

jonasschnelli deleted a comment on Apr 18, 2018

Add priority block request queue 9946ff7734

Add new ProcessPriorityRequest main signal f2b165fb6e

Don't ActivateBestChain when processing priority block request c2e6e7779c

Process priority requests 321b1f78b3

Optionally allow to skip the toFarAway check in AcceptBlock 65cf118879

Add fAutoRequestBlocks to disabled/enable the verification progress b9ebf85827

[RPC] Add requestblocks - a simple way to priorize block downloads 5263167758

Add -autorequestblocks which then allows to run in non-validation mode (pure light-client mode) 70458206c0

jonasschnelli force-pushed on Apr 18, 2018

jonasschnelli commented at 8:15 am on April 18, 2018: contributor

Rebased

jonasschnelli force-pushed on Apr 18, 2018

[QA] add requestblocks test 2326318a95

Pass priority requests through ZMQ notifications 77561f7a4b

jonasschnelli force-pushed on Apr 18, 2018

MarcoFalke added the label Needs rebase on Jun 6, 2018

Sjors commented at 6:55 pm on August 2, 2018: member

I rebased it here: https://github.com/Sjors/bitcoin/commits/2018/08/spv-rpc (I may have broken the functional test) @ryanofsky said:

I still can’t figure out if new RPC is actually supposed to be useful for something other than debugging/testing

The bigger picture for me is #9483, the ability for users to get started while IBD happens in the background.

However, a use case I have in mind for requestblocks add is https://github.com/ElementsProject/lightning/issues/1297, in particular the ability for a lightning client to re-download historic blocks from a pruned node. This actually works now. Do they get pruned again at some point?

-autorequestblocks is useful when running on a phone (iOs: #11720, Android: ABCore). A native application could use the RPC and only allow fetching blocks when on wifi. Being able to toggle automatic download via RPC would be nice, but can wait.

Being able to fetch blocks by height range (or up to a certain height) instead of hashes would be nice, but that’s something that can be built on top of this.

I noticed there’s no log entry when you requestblocks add.

Can you explain when priorityRequest is true?

Did some light testing and couldn’t find problems, but more people should try :-)

in test/functional/rpc_requestblocks.py:59 in 77561f7a4b

54+        block = self.nodes[1].getblock(node0bbhash, True)
55+        assert_equal(block['hash'], node0bbhash)
56+        assert_equal(block['validated'], False)
57+
58+        #prevblock must not be available
59+        assert_raises_rpc_error(-1, "Block not found on disk", self.nodes[1].getblock, block['previousblockhash'])

Sjors commented at 7:01 pm on August 2, 2018:

Maybe do a prune here and then refetch the old block?

in src/rpc/blockchain.cpp:1683 in 77561f7a4b

1678+
1679+        AddPriorityDownload(blocksToDownload);
1680+        return NullUniValue;
1681+    }
1682+    else {
1683+        throw JSONRPCError(RPC_INVALID_PARAMETER, "Unkown action");

practicalswift commented at 6:26 pm on September 2, 2018:

Typo found by codespell: Unkown

DrahtBot added the label Up for grabs on Dec 3, 2018

DrahtBot commented at 4:54 pm on December 3, 2018: member

DrahtBot closed this on Dec 3, 2018

laanwj removed the label Needs rebase on Oct 24, 2019

DrahtBot locked this on Dec 16, 2021

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

Simple RPC App Example

Other Concerns

1

2