[WIP][Experimental] Add Hybrid full block SPV mode #9076

jonasschnelli commented at 8:03 PM on November 3, 2016: contributor

work in progress, seeks conceptual review very experimental right now

This adds a hybrid and pure SPV mode to bitcoin cores wallet (including GUI). The main out-of-band block requests are handled over the new "lock-free" class CBlockRequest. This is a generic interface to allow retrieving a range of blocks (only headers PoW check, no validation) and throw them through the SyncTransaction() signals (could also be useful for ZMQ, etc,).

Hybrid mode

Can be enabled by -spv. If enabled, the wallet tries to download blocks from the depth where the wallet was created (oldest key-birthday). Those requested blocks are getting prioritized in FindNextBlocksToDownload(). Once the next block (in order/sequence) has been retrieved, it gets sent through the SyncTransaction signal. Those - non-verified transactions will get a spv=true flag (visible in the GUI as well as over RPC). The blocks are kept on the disk for later verification. In the background, the IBD/verification is still processing/downloading blocks. Once a transaction was verified by connecting/verifying its block, the SPV flag gets removed from the transaction.

This result in a hybrid full-block SPV mode where the users doesn't need to download more (they later need to verify the blocks near the tip anyways) but its possible to already see incoming transactions and send out coins

Pure SPV

Can be enabled by -spvonly. Result in not downloading blocks for verification.

Current limitations

No SPV 0-conf transactions (those are pretty unsafe anyways)
Fallback fee for SPV transaction (missing mempool/fee estimator)
It has only a simple spv re-org handing
currently incompatible with pruning

Thanks for feedback.

jonasschnelli added the label Wallet on Nov 3, 2016

ryanofsky commented at 10:41 PM on November 3, 2016: member

Possibly dumb question but to be sure I am understanding the change correctly: In pure SPV mode, this will still download and store full blocks for everything after the oldest key-birthday?

jonasschnelli commented at 7:21 AM on November 4, 2016: contributor

Possibly dumb question but to be sure I am understanding the change correctly: In pure SPV mode, this will still download and store full blocks for everything after the oldest key-birthday?

Yes. A next step could be to support BIP37 bloom filter against authenticated nodes (once we have BIP150). BIP37 has huge privacy implication while this PRs full block SPV mode does not.

Right now the -spvonly mode is not very bandwidth efficient (if you assume you'll never want to do the full validation of the chain).

jonasschnelli force-pushed on Nov 4, 2016

jonasschnelli commented at 9:39 AM on November 4, 2016: contributor

getwalletinfo reports the SPV sync state:

{
  "walletversion": 130000,
  "balance": 10.00000000,
  "unconfirmed_balance": 0.00000000,
  "immature_balance": 0.00000000,
  "txcount": 1,
  "keypoololdest": 1478202465,
  "keypoolsize": 10,
  "paytxfee": 0.00000010,
  "hdmasterkeyid": "fc2f268240531df6890b66f53141606b7f1f091b",
  "spv": {
    "enabled": true,
    "hybrid_mode": false,
    "synced_up_to_height": 102,
    "best_known_header_height": 102,
    "sync_in_progress": false
  }
}

If a sync is in progress, it can look like this:

{
  "walletversion": 130000,
  "balance": 0.00010000,
  "unconfirmed_balance": 0.00000000,
  "immature_balance": 0.00000000,
  "txcount": 1,
  "keypoololdest": 1477594751,
  "keypoolsize": 100,
  "paytxfee": 0.00000000,
  "hdmasterkeyid": "b397c61fb9d18e24d3a42f9c3c7b5d88b15005d3",
  "spv": {
    "enabled": true,
    "hybrid_mode": true,
    "synced_up_to_height": 436884,
    "best_known_header_height": 437299,
    "sync_in_progress": true,
    "started": 1478252299,
    "is_cancled": false,
    "requested_blocks": 416,
    "loaded_blocks": 4,
    "processed_blocks": 1
  }
}

jonasschnelli force-pushed on Nov 4, 2016

ryanofsky commented at 2:37 PM on November 4, 2016: member

What are major things that need to be done to finalize this change? Are making hybrid SPV mode compatible with pruning, and improving reorg handling critical parts of this change, or would they be future improvements? Looking through the code, there are lots of minor things I could comment on, but I'm assuming I should hold off if you're mainly looking for "conceptual review" right now from core developers (the concept does seems great to me).

I do have one piece of feedback, though. Based on my understanding of the change, I think the following flag / feature descriptions might be more comprehensible to users:

prioritized block download (instead of "hybrid SPV") - When enabled, bitcoin downloads the full blockchain as usual, but prioritizes downloading of newer blocks (currently all blocks after the earliest wallet transaction date), before older blocks. This causes relevant transactions to show up in the wallet earlier during an initial sync, allowing the wallet to become functional more quickly, but at the cost of revealing some information about the age of addresses in the wallet to peers on the network.
partial sync (instead of "full SPV") - When enabled, bitcoin downloads only part of the blockchain (currently all blocks after the earliest wallet transaction date), but still downloads and stores full blocks. This means less of the blockchain needs to be downloaded and stored locally, but at the cost of not being able to fully validate transactions, and of revealing some information about the age of addresses in the wallet to peers on the network. This mode is also is incompatible with the -prune block pruning mode (since bitcoin can't compute the UTXO set), so a node with partial syncing enabled may wind up using more disk space than a node with pruning enabled, despite downloading less data over the network.

I think I like these names more than "hybrid SPV" / "full SPV", just because I think the modern usage of the term "SPV" refers to the wallets that don't download full blocks, don't validate transactions, barely require any storage, and sync almost immediately, and this is not what is implemented in these new modes. I do understand that historically the term SPV refered to a wider variety of network/storage saving schemes, but I don't think it would be good to go back and muddy the more clear modern meaning of what an SPV wallet is.

jonasschnelli commented at 2:46 PM on November 4, 2016: contributor

What are major things that need to be done to finalize this change? Are making hybrid SPV mode compatible with pruning, and improving reorg handling critical parts of this change, or would they be future improvements? Looking through the code, there are lots of minor things I could comment on, but I'm assuming I should hold off if you're mainly looking for "conceptual review" right now from core developers (the concept does seems great to me).

Feels free to comment on the minor things directly in the code. I don't expect that this will go into master as it is (to big, includes to many risks, lack of reviewers). As always, I'll try to split this into independent sub-PRs. This one is to get an fully running PoC and maybe discuss it on a higher level.

prioritized block download

Yes. Seems fine for me. But we just need to make sure that users know that no validation happens on the "prioritized blocks".

partial sync

Yes. Though I think an expected feature would be to drop the part where we store blocks on the disk when using -spvonly. And I'm aiming to use some kind of BIP37 mechanism to allow low-bandwidth transaction scan against a trusted full node (after BIP150/151). Maybe we need to choose the wording with that in mind.

in qa/rpc-tests/walletspv.py:None in eb13afe4de outdated

  19 | +
  20 | +        self.utxo = []
  21 | +        self.txouts = gen_return_txouts()
  22 | +        self.extra_args = [["-debug=net"], ["-spv=1", "-spvonly=1", "-debug=net"], ["-debug=net"]]
  23 | +
  24 | +    def setup_network(self, split=False):

ryanofsky commented at 2:56 PM on November 4, 2016:

Remove unused split argument?

in qa/rpc-tests/walletspv.py:None in eb13afe4de outdated

   5 | +
   6 | +from test_framework.mininode import *
   7 | +from test_framework.test_framework import BitcoinTestFramework
   8 | +from test_framework.util import *
   9 | +import time
  10 | +from pprint import pprint

ryanofsky commented at 2:56 PM on November 4, 2016:

Seems unused

in qa/rpc-tests/walletspv.py:None in eb13afe4de outdated

  13 | +class WalletSPVTest(BitcoinTestFramework):
  14 | + 
  15 | +    def __init__(self):
  16 | +        super().__init__()
  17 | +        self.setup_clean_chain = True
  18 | +        self.num_nodes = 4

ryanofsky commented at 2:59 PM on November 4, 2016:

Can this be dropped? Number of nodes seems to change during the test.

in qa/rpc-tests/walletspv.py:None in eb13afe4de outdated

  15 | +    def __init__(self):
  16 | +        super().__init__()
  17 | +        self.setup_clean_chain = True
  18 | +        self.num_nodes = 4
  19 | +
  20 | +        self.utxo = []

ryanofsky commented at 3:00 PM on November 4, 2016:

Unused?

in qa/rpc-tests/walletspv.py:None in eb13afe4de outdated

  53 | +        self.nodes[0].generate(130)
  54 | +        self.nodes[0].sendtoaddress(addr, 1.1)
  55 | +        self.nodes[0].generate(1)
  56 | +        headerheight = self.nodes[0].getblockchaininfo()['headers']
  57 | +        self.sync_spv(headerheight)
  58 | +        time.sleep(5)

ryanofsky commented at 3:13 PM on November 4, 2016:

Sleep probably deserves comment if sync_spv and sync_blocks (in util.py) aren't enough to be able to remove it.

in qa/rpc-tests/walletspv.py:None in eb13afe4de outdated

  68 | +        # txes should not be present in the wallet with spv
  69 | +        assert_equal(lt[0]['address'], addr)
  70 | +        assert_equal(lt[0]['spv'], True)
  71 | +        assert_equal(lt[0]['confirmations'], 1)
  72 | +        
  73 | +        print("Restarting nodes without -spvonly (hybrid SPV)")

ryanofsky commented at 3:21 PM on November 4, 2016:

Curious, why stop 1st node here, instead of just starting a 4th node? Is it to make sure the wallet file is synced to disk? Probably good to have a little comment here describing this part of the test.

in qa/rpc-tests/walletspv.py:None in eb13afe4de outdated

  60 | +        node1info = self.nodes[1].getblockchaininfo()
  61 | +        assert_equal(node1info['blocks'], 0)
  62 | +        assert_equal(node1info['headers'], headerheight)
  63 | +        spvinfo = self.nodes[1].getwalletinfo()['spv']
  64 | +        assert_equal(headerheight, spvinfo['best_known_header_height'])
  65 | +        lt = self.nodes[1].listtransactions()

ryanofsky commented at 3:23 PM on November 4, 2016:

Check len(lt) is 1?

in qa/rpc-tests/walletspv.py:None in eb13afe4de outdated

  79 | +        connect_nodes_bi(self.nodes,1, 2)
  80 | +    
  81 | +        self.nodes[0].resendwallettransactions()
  82 | +        self.sync_all()
  83 | +        
  84 | +        lt = self.nodes[1].listtransactions()

ryanofsky commented at 3:24 PM on November 4, 2016:

Check len(lt)?

in qa/rpc-tests/walletspv.py:None in eb13afe4de outdated

  97 | +        self.sync_spv(headerheight)
  98 | +        # make sure we have identical mempools
  99 | +        self.nodes[0].resendwallettransactions()
 100 | +        self.sync_all()
 101 | +        
 102 | +        # currently there is no efficient way to test for the "SPV first" wallet listing

ryanofsky commented at 3:32 PM on November 4, 2016:

Not sure, but maybe it's possible to test this by starting up a new hybrid SPV node with a wallet and no chain data and seeing which blocks it first requests using a stub peer. The test_getblocktxn_requests and test_compactblock_requests tests in p2p-compactblocks.py do something like this using a custom TestNode peer.

in qa/rpc-tests/walletspv.py:None in eb13afe4de outdated

 100 | +        self.sync_all()
 101 | +        
 102 | +        # currently there is no efficient way to test for the "SPV first" wallet listing
 103 | +        # but we test that the hybrid modes results with a standard full validated wtxns
 104 | +        lt = self.nodes[3].listtransactions()
 105 | +        assert_equal(lt[0]['address'], addr)

ryanofsky commented at 3:32 PM on November 4, 2016:

Check len(lt)?

in src/blockrequest.cpp:None in eb13afe4de outdated

  11 | +
  12 | +std::shared_ptr<CBlockRequest> currentBlockRequest; //multithread save pointer (CBlockRequest, the object, is also lock-free)
  13 | +
  14 | +CBlockRequest::CBlockRequest(std::vector<CBlockIndex*> vBlocksToDownloadIn, int64_t createdIn, const std::function<bool(std::shared_ptr<CBlockRequest>, CBlockIndex *pindex)> progressCallbackIn) : vBlocksToDownload(vBlocksToDownloadIn), created(createdIn), progressCallback(progressCallbackIn)
  15 | +{
  16 | +    fCancled = false;

ryanofsky commented at 3:33 PM on November 4, 2016:

Spelling of cancelled.

in src/blockrequest.cpp:None in eb13afe4de outdated

   7 | +#include "chainparams.h"
   8 | +#include "main.h"
   9 | +
  10 | +#include <exception>
  11 | +
  12 | +std::shared_ptr<CBlockRequest> currentBlockRequest; //multithread save pointer (CBlockRequest, the object, is also lock-free)

ryanofsky commented at 3:34 PM on November 4, 2016:

s/multithread save/thread-safe

in src/blockrequest.cpp:None in eb13afe4de outdated

  13 | +
  14 | +CBlockRequest::CBlockRequest(std::vector<CBlockIndex*> vBlocksToDownloadIn, int64_t createdIn, const std::function<bool(std::shared_ptr<CBlockRequest>, CBlockIndex *pindex)> progressCallbackIn) : vBlocksToDownload(vBlocksToDownloadIn), created(createdIn), progressCallback(progressCallbackIn)
  15 | +{
  16 | +    fCancled = false;
  17 | +    requestedUpToSize = 0;
  18 | +    processedUpToSize = 0;

ryanofsky commented at 3:52 PM on November 4, 2016:

Since bitcoin is using c++11 can you just initialize these in the header?

in src/blockrequest.cpp:None in eb13afe4de outdated

  94 | +    currentBlockRequest = shared_from_this();
  95 | +}
  96 | +
  97 | +void CBlockRequest::fillInNextBlocks(std::vector<CBlockIndex*>& vBlocks, unsigned int count, std::function<bool(CBlockIndex*)> filterBlocksCallback)
  98 | +{
  99 | +    for (unsigned int i = this->processedUpToSize; i < this->vBlocksToDownload.size() ; i++) {

ryanofsky commented at 4:05 PM on November 4, 2016:

Should be size_t

in src/blockrequest.cpp:None in eb13afe4de outdated

  97 | +void CBlockRequest::fillInNextBlocks(std::vector<CBlockIndex*>& vBlocks, unsigned int count, std::function<bool(CBlockIndex*)> filterBlocksCallback)
  98 | +{
  99 | +    for (unsigned int i = this->processedUpToSize; i < this->vBlocksToDownload.size() ; i++) {
 100 | +        CBlockIndex *pindex = this->vBlocksToDownload[i];
 101 | +        if ( filterBlocksCallback(pindex) && !(pindex->nStatus & BLOCK_HAVE_DATA)) {
 102 | +            // we don't already download this block and we don't have its data already

rebroad commented at 4:20 PM on November 4, 2016:

this is not quite right English and I don't understand what it is saying. Do you mean "we haven't already downloaded this block nor do we have its data"?

"we don't" tends to mean "we should not".

jonasschnelli commented at 2:29 PM on November 8, 2016:

Thanks, will change.

in src/blockrequest.cpp:None in eb13afe4de outdated

  99 | +    for (unsigned int i = this->processedUpToSize; i < this->vBlocksToDownload.size() ; i++) {
 100 | +        CBlockIndex *pindex = this->vBlocksToDownload[i];
 101 | +        if ( filterBlocksCallback(pindex) && !(pindex->nStatus & BLOCK_HAVE_DATA)) {
 102 | +            // we don't already download this block and we don't have its data already
 103 | +            vBlocks.push_back(pindex);
 104 | +            if (vBlocks.size() == count) {

ryanofsky commented at 4:48 PM on November 4, 2016:

Maybe instead of a break just add vBlocks.size() < count as a condition in the for loop. I think that would make it clearer what count is supposed to represent (maximum number of blocks ever allowed in vBlocks as opposed to some limit associated with block state).

in src/blockrequest.cpp:None in eb13afe4de outdated

 106 | +            }
 107 | +        }
 108 | +    }
 109 | +
 110 | +    //try to push already available blocks through the signal
 111 | +    this->processWithPossibleBlock(NULL, NULL);

ryanofsky commented at 4:52 PM on November 4, 2016:

Can omit nulls since this defines default arguments.

in src/blockrequest.h:None in eb13afe4de outdated

  27 | +
  28 | +    /** Process the request, check if there are blocks available to "stream"
  29 | +        over the SyncTransaction signal 
  30 | +        Allow to provide an optional block to avoid disk re-loading
  31 | +     */
  32 | +    void processWithPossibleBlock(const CBlock* pblock = NULL, CBlockIndex *pindex = NULL);

ryanofsky commented at 4:59 PM on November 4, 2016:

Maybe call this something like SyncDownloadedTransactions. Since the purpose of this function is to invoke the SyncTransaction signal, seems like it should be named similarly to the signal.

dooglus commented at 5:00 PM on November 4, 2016: contributor

@ryanofsky

but at the cost of revealing some information about the age of addresses in the wallet to peers on the network

The bigger cost is that the wallet could end up downloading a completely fake chain, since it isn't anchored to the genesis block. Presumably the client has no idea what block height or difficulty to expect and so evil peers could feed the SPV client minimum difficulty blocks of their own construction containing transactions that don't exist on the main chain.

in src/blockrequest.cpp:None in eb13afe4de outdated

  78 | +        // release shared pointer
  79 | +        currentBlockRequest = nullptr;
  80 | +    }
  81 | +}
  82 | +
  83 | +bool CBlockRequest::isCancled()

ryanofsky commented at 5:04 PM on November 4, 2016:

Should be const (the member function).

in src/blockrequest.cpp:None in eb13afe4de outdated

  92 | +        currentBlockRequest->fCancled = true;
  93 | +
  94 | +    currentBlockRequest = shared_from_this();
  95 | +}
  96 | +
  97 | +void CBlockRequest::fillInNextBlocks(std::vector<CBlockIndex*>& vBlocks, unsigned int count, std::function<bool(CBlockIndex*)> filterBlocksCallback)

ryanofsky commented at 5:08 PM on November 4, 2016:

Should be const, I think.

Also might rename fillInNextBlocks to getNextBlocks to be clearer that this is returning information about the next blocks in the request, not changing the next blocks in the request.

in src/blockrequest.cpp:None in eb13afe4de outdated

 109 | +
 110 | +    //try to push already available blocks through the signal
 111 | +    this->processWithPossibleBlock(NULL, NULL);
 112 | +}
 113 | +
 114 | +unsigned int CBlockRequest::amountOfBlocksLoaded()

ryanofsky commented at 5:10 PM on November 4, 2016:

Should be const, and probably use size_t. Also maybe change "loaded" in the name to "downloaded" or "have data" to be consistent with other names here ("loaded" by itself is kind of vague).

in src/blockrequest.cpp:None in eb13afe4de outdated

 112 | +}
 113 | +
 114 | +unsigned int CBlockRequest::amountOfBlocksLoaded()
 115 | +{
 116 | +    unsigned int haveData = 0;
 117 | +    for (unsigned int i = 0; i < this->vBlocksToDownload.size() ; i++) {

ryanofsky commented at 5:10 PM on November 4, 2016:

Should be size_t

in src/blockrequest.cpp:None in eb13afe4de outdated

   9 | +
  10 | +#include <exception>
  11 | +
  12 | +std::shared_ptr<CBlockRequest> currentBlockRequest; //multithread save pointer (CBlockRequest, the object, is also lock-free)
  13 | +
  14 | +CBlockRequest::CBlockRequest(std::vector<CBlockIndex*> vBlocksToDownloadIn, int64_t createdIn, const std::function<bool(std::shared_ptr<CBlockRequest>, CBlockIndex *pindex)> progressCallbackIn) : vBlocksToDownload(vBlocksToDownloadIn), created(createdIn), progressCallback(progressCallbackIn)

ryanofsky commented at 5:33 PM on November 4, 2016:

Could do vBlocksToDownload(std::move(vBlocksToDownloadIn)) to avoid copying vector.

in src/main.cpp:None in eb13afe4de outdated

  88 | @@ -88,6 +89,8 @@ CAmount maxTxFee = DEFAULT_TRANSACTION_MAXFEE;
  89 |  CTxMemPool mempool(::minRelayTxFee);
  90 |  FeeFilterRounder filterRounder(::minRelayTxFee);
  91 |  
  92 | +std::atomic<bool> fAutodownloadBlocks(true);

ryanofsky commented at 5:38 PM on November 4, 2016:

Would be nice to name the variable after the command line argument it comes from (currently "-spvonly", possibly something else like "partial sync" in the future).

in src/main.cpp:None in eb13afe4de outdated

 213 | @@ -211,6 +214,7 @@ namespace {
 214 |          CBlockIndex* pindex;                                     //!< Optional.
 215 |          bool fValidatedHeaders;                                  //!< Whether this block has validated headers at the time of request.
 216 |          std::unique_ptr<PartiallyDownloadedBlock> partialBlock;  //!< Optional, used for CMPCTBLOCK downloads
 217 | +        std::shared_ptr<CBlockRequest> blockRequest;                  //!< Optional, used for specific block downloads (SPV)

ryanofsky commented at 5:40 PM on November 4, 2016:

Extra whitespace.

in src/main.cpp:None in eb13afe4de outdated

 572 | @@ -562,6 +573,26 @@ void FindNextBlocksToDownload(NodeId nodeid, unsigned int count, std::vector<CBl
 573 |      // Make sure pindexBestKnownBlock is up to date, we'll need it.
 574 |      ProcessBlockAvailability(nodeid);
 575 |  
 576 | +    // if there is an open CBlockRequest (out-of-band/specific block donwload), privileg it
 577 | +    if (blockRequest && !blockRequest->isCancled()) {
 578 | +        // fill in next blocks to download, pass in a filter function to check mapBlocksInFlight

ryanofsky commented at 5:52 PM on November 4, 2016:

Maybe s/check mapBlocksInFlight/exclude blocks in flight/ to be more specific. (I had thought it was doing the opposite the first time reading this).

in src/main.cpp:None in eb13afe4de outdated

 868 | @@ -838,8 +869,7 @@ bool CheckFinalTx(const CTransaction &tx, int flags)
 869 |      // evaluated is what is used. Thus if we want to know if a
 870 |      // transaction can be part of the *next* block, we need to call
 871 |      // IsFinalTx() with one more than chainActive.Height().
 872 | -    const int nBlockHeight = chainActive.Height() + 1;
 873 | -
 874 | +    const int nBlockHeight = ((fHeadersChain && pindexBestHeader) ? pindexBestHeader->nHeight : chainActive.Height()) + 1;

ryanofsky commented at 6:31 PM on November 4, 2016:

Relying on this fHeadersChain argument here is bugging me (the argument is optional and defaults to false), since it seems easy for callers to screw up. I think it would be best to either make it a mandatory argument and/or provide some overload like the following, that would pass in the right value automatically for callers:

bool CheckFinal(const CMerkleTx& mtx, int flags=-1)
{
  return CheckFinal(mtx, flags, mtx.fSPV);
}

ryanofsky commented at 6:52 PM on November 4, 2016: member

Only about halfway through the diffs, but here are my comments so far (all minor).

ghost commented at 10:02 PM on November 4, 2016: none

We've done this 6 months ago in Vcash and will be deploying it soon (ZeroLedger). With the extensions we call it SPV+. Let us know if we can help. 👍

jonasschnelli commented at 9:34 PM on November 5, 2016: contributor

@ryanofsky Thanks for the feedback! Will work myself through it next week.

but at the cost of revealing some information about the age of addresses in the wallet to peers on the network

Why would your peer reveal information about the age of his keys? We are downloading blocks from different peers and I don't see a possibility to meaningful track ages of the wallets keys because they way of how we retrieve blocks in identical to how we retrieve blocks in standard IBD. @dooglus

The bigger cost is that the wallet could end up downloading a completely fake chain, since it isn't anchored to the genesis block. Presumably the client has no idea what block height or difficulty to expect and so evil peers could feed the SPV client minimum difficulty blocks of their own construction containing transactions that don't exist on the main chain.

Because we already have downloaded and built the headers-only-chain, all "SPV" downloaded blocks are linked to the genesis block. Though, not through verification, only through a simple PoW/chainwork check. And there are also the checkpoints (though not sure how long). But yes. The risk of getting sybilled of the main chain during the headers-sync can cause more pain in SPV mode.

rebroad commented at 1:35 PM on November 6, 2016: contributor

@john-connor can you post a link to any code you think would be useful please?

in src/main.cpp:None in eb13afe4de outdated

3057 | @@ -3028,6 +3058,7 @@ static void NotifyHeaderTip() {
3058 |      // Send block tip changed notifications without cs_main
3059 |      if (fNotify) {
3060 |          uiInterface.NotifyHeaderTip(fInitialBlockDownload, pindexHeader);
3061 | +        GetMainSignals().UpdatedBlockHeaderTip(fInitialBlockDownload, pindexHeader);

ryanofsky commented at 2:55 PM on November 7, 2016:

Maybe this could be called NotifyHeaderTip instead of UpdatedBlockHeaderTip? It doesn't seem good to have two different but confusingly similar names for this.

in src/main.cpp:None in eb13afe4de outdated

6929 |              BOOST_FOREACH(CBlockIndex *pindex, vToDownload) {
6930 |                  uint32_t nFetchFlags = GetFetchFlags(pto, pindex->pprev, consensusParams);
6931 |                  vGetData.push_back(CInv(MSG_BLOCK | nFetchFlags, pindex->GetBlockHash()));
6932 | -                MarkBlockAsInFlight(pto->GetId(), pindex->GetBlockHash(), consensusParams, pindex);
6933 | -                LogPrint("net", "Requesting block %s (%d) peer=%d\n", pindex->GetBlockHash().ToString(),
6934 | +                list<QueuedBlock>::iterator *queuedBlockIt = NULL;

ryanofsky commented at 6:36 PM on November 7, 2016:

New variable seems to be unused. Could you continue to pass null to MarkBlockAsInFlight instead of this? Probably deserves comment if this variable is actually being used in some unseen way.

in src/validationinterface.h:None in eb13afe4de outdated

  31 | @@ -32,7 +32,9 @@ void UnregisterAllValidationInterfaces();
  32 |  class CValidationInterface {
  33 |  protected:
  34 |      virtual void UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload) {}
  35 | -    virtual void SyncTransaction(const CTransaction &tx, const CBlockIndex *pindex, int posInBlock) {}
  36 | +    virtual void UpdatedBlockHeaderTip(bool fInitialDownload, const CBlockIndex *pindexNew) {}
  37 | +    virtual void SyncTransaction(const CTransaction &tx, const CBlockIndex *pindex, int posInBlock, bool validated = true) {}

ryanofsky commented at 6:47 PM on November 7, 2016:

Having validated default true seems less safe than having validated default false, or not having a default at all.

Also it seems like validated == !fSpv. Seems like this argument could be changed to an fSpv, or the other fSpv's could be changed to validated variables for more consistency.

in src/wallet/wallet.cpp:None in eb13afe4de outdated

  60 | @@ -59,6 +61,12 @@ CFeeRate CWallet::fallbackFee = CFeeRate(DEFAULT_FALLBACK_FEE);
  61 |  
  62 |  const uint256 CMerkleTx::ABANDON_HASH(uint256S("0000000000000000000000000000000000000000000000000000000000000001"));
  63 |  
  64 | +std::atomic<bool> fUseSPV(true);
  65 | +std::atomic<bool> fSPVOnly(true);

ryanofsky commented at 6:49 PM on November 7, 2016:

Variable is unused.

in src/wallet/wallet.cpp:None in eb13afe4de outdated

3454 | @@ -3352,6 +3455,8 @@ std::string CWallet::GetWalletHelpString(bool showDebug)
3455 |      if (showDebug)
3456 |          strUsage += HelpMessageOpt("-sendfreetransactions", strprintf(_("Send transactions as zero-fee transactions if possible (default: %u)"), DEFAULT_SEND_FREE_TRANSACTIONS));
3457 |      strUsage += HelpMessageOpt("-spendzeroconfchange", strprintf(_("Spend unconfirmed change when sending transactions (default: %u)"), DEFAULT_SPEND_ZEROCONF_CHANGE));
3458 | +    strUsage += HelpMessageOpt("-spv", strprintf(_("Use hybrid SPV mode to show transactions before verification (default: %u)"), DEFAULT_ENABLE_HYBRID_SPV));
3459 | +    strUsage += HelpMessageOpt("-spvonly", strprintf(_("Use hybrid SPV mode to show transactions before verification (default: %u)"), DEFAULT_ENABLE_HYBRID_SPV));

ryanofsky commented at 6:52 PM on November 7, 2016:

Should be DEFAULT_ENABLE_PURE_SPV

in src/wallet/wallet.cpp:None in eb13afe4de outdated

3749 | +        LogPrintf("%s: parameter interaction: -spvonly=1 -> setting -spv=1\n", __func__);
3750 | +    }
3751 | +
3752 | +    // if using block pruning, then disallow spv
3753 | +    if (GetArg("-prune", 0)) {
3754 | +        if (GetBoolArg("-spv", DEFAULT_ENABLE_HYBRID_SPV) || GetBoolArg("-spvonly", DEFAULT_ENABLE_HYBRID_SPV))

ryanofsky commented at 6:52 PM on November 7, 2016:

Should be DEFAULT_ENABLE_PURE_SPV (for -spvonly)

in src/wallet/wallet.cpp:None in eb13afe4de outdated

1170 | +        LOCK2(cs_main, cs_wallet);
1171 | +        bestSPVBlockHeight = nBestSpvHeight;
1172 | +    }
1173 | +
1174 | +    // check if the new tip extends the wallets current known best headers tip
1175 | +    const CBlockIndex *pCurrent = pindexNew;

ryanofsky commented at 8:00 PM on November 7, 2016:

Can pindexNew ever be null? If if it is null, should fReOrg always be true in this case, or should fReorg only be true if bestSpvBlockHash is null? Comment might be useful to clarify.

If pIndexNew should never be null, would suggest either asserting this or changing the while to a do-while, and removing the "pindexNew && ..." check below.

in src/wallet/wallet.cpp:None in eb13afe4de outdated

1196 | +            CWalletTx* wtx = &(*it).second;
1197 | +            if (wtx->fSPV)
1198 | +            {
1199 | +                bool found = false;
1200 | +                const CBlockIndex *pCurrent = pindexNew;
1201 | +                while (pCurrent)

ryanofsky commented at 8:02 PM on November 7, 2016:

This while loop duplicates the loop above (even masking the pCurrent variable), maybe both loops could be replaced with a new FindPrevBlockWithHash(uint256) function call.

in src/wallet/wallet.cpp:None in eb13afe4de outdated

1192 | +        int64_t rescanBackTo = std::numeric_limits<int64_t>::max();
1193 | +        // the current wallets best header tip is not in the active headers chain
1194 | +        for (map<uint256, CWalletTx>::iterator it = mapWallet.begin(); it != mapWallet.end(); ++it)
1195 | +        {
1196 | +            CWalletTx* wtx = &(*it).second;
1197 | +            if (wtx->fSPV)

ryanofsky commented at 8:09 PM on November 7, 2016:

Why is it significant that the txn is SPV? Isn't any transaction that's been reorged away be no longer confirmed? Might be good to have a comment on this here.

in src/wallet/wallet.cpp:None in eb13afe4de outdated

3765 | +{
3766 | +    CBlockIndex *pIndex = NULL;
3767 | +    CBlockIndex *chainActiveTip = NULL;
3768 | +    // calculate the oldest key and don't use nTimeFirstKey
3769 | +    // adding WatchOnly addresses will result in nTimeFirstKey == 1
3770 | +    int64_t oldest_key = std::numeric_limits<int64_t>::max();;

ryanofsky commented at 8:11 PM on November 7, 2016:

;

in src/wallet/wallet.h:None in eb13afe4de outdated

 628 | @@ -616,7 +629,9 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
 629 |      typedef std::map<unsigned int, CMasterKey> MasterKeyMap;
 630 |      MasterKeyMap mapMasterKeys;
 631 |      unsigned int nMasterKeyMaxID;
 632 | -
 633 | +    int nBestSpvHeight;

ryanofsky commented at 8:14 PM on November 7, 2016:

It'd be good to say what best means in a comment.

in src/wallet/wallet.cpp:None in eb13afe4de outdated

1218 | +                }
1219 | +            }
1220 | +        }
1221 | +        // try to find a reasonable height to spv-scan from again
1222 | +        const CBlockIndex *pCurrent = pindexNew;
1223 | +        nBestSpvHeight = 0;

ryanofsky commented at 8:22 PM on November 7, 2016:

Above you are acquiring a lock when you read the nBestSpvHeight variable. Is it not necessary to have a lock when writing the same variable?

in src/wallet/wallet.cpp:None in eb13afe4de outdated

1233 | +            pCurrent = pCurrent->pprev;
1234 | +        }
1235 | +        if (nBestSpvHeight > 0 && rescanBackTo != std::numeric_limits<int64_t>::max())
1236 | +            ScanSPV(rescanBackTo);
1237 | +    }
1238 | +    if (fUseSPV && pindexNew && pindexNew->nHeight > bestSPVBlockHeight)

ryanofsky commented at 8:24 PM on November 7, 2016:

Is this actually supposed to be bestSPVBlockHeight not nBestSpvHeight? Why compare against previous height instead of current? If this is intended would suggest adding a comment to explain and also renaming bestSPVBlockHeight to previousBestSPVBlockHeight to make clear this is supposed to be the previous height.

in src/wallet/wallet.cpp:None in eb13afe4de outdated

1643 | @@ -1544,7 +1644,7 @@ bool CWalletTx::RelayWalletTransaction(CConnman* connman)
1644 |      assert(pwallet->GetBroadcastTransactions());
1645 |      if (!IsCoinBase())
1646 |      {
1647 | -        if (GetDepthInMainChain() == 0 && !isAbandoned() && InMempool()) {
1648 | +        if (GetDepthInMainChain() == 0 && !isAbandoned() && (InMempool() || fSPV)) {

ryanofsky commented at 8:45 PM on November 7, 2016:

I don't understand in general what the resend wallet transaction code is for, and probably this isn't the place to explain it, but it might be nice to have a comment here saying how the SPV condition relates.

jonasschnelli force-pushed on Nov 8, 2016

[WIP][Experimental] Add Hybrid SPV mode 96bdeffb78

jonasschnelli force-pushed on Nov 8, 2016

jonasschnelli commented at 7:31 AM on November 9, 2016: contributor

Binaries are available here: https://bitcoin.jonasschnelli.ch/pulls/9076/

mruddy commented at 11:29 AM on November 28, 2016: contributor

@jonasschnelli Conceptually, I'm not for putting the ability to change the node's security model into Core's code base right now. To me, this client should target being a fully validating node. Otherwise, the code will become more complex and difficult to reason about. I hope this does not come across as throwing stones because that's not how I mean it. Coincidentally, I thought a lot about this recently due to work on #9180. That PR effectively created another flavor of a non-fully validating hybrid mode and the feedback in that PR may apply here. If nothing else, this comment will link in that work on another potential hybrid mode possibility if you hadn't seen it already.

sipa commented at 7:18 PM on November 28, 2016: member

@mruddy That's a fair view, and probably one that is shared by some contributors. Let me offer some other perspective, though.

I believe that conceptually, in the long term, the Bitcoin Core wallet should be more isolated and separated from the node/consensus logic. I'm unsure what this will mean for organization of code base, but at least it should run as a separate process for security reasons. There is ongoing discussion on how the wallet and node processes should interact, but my view is that they should just use the P2P protocol. The P2P protocol was designed for supporting light wallets, and our server side implementation of it is well tested for this purpose. Apart from questions about mempool tracking and feerate estimation, there is effectively no server side work to accomplish this.

I see this PR as a step towards the client side. It's just allowing the wallet code to run with consensus enforcement done in another process. It's different from the checkpoints discussion in that it doesn't result in a node that acts like a full node.

mruddy commented at 2:03 AM on November 29, 2016: contributor

@sipa Thanks for the additional insight. I can see that. The node security model is distinct from the wallet security model. The longer term goal of wallet-node separation seems appealing.

I'm still concerned about keeping complexity down while maintaining these logical separations in a single repo.

For example, do you think it would be valid feedback for the current patch that changes to ProcessNewBlock and AcceptBlock violate wallet-node code level separation?

If the interface between the processes is the P2P protocol, then I think the difference should be handled as separate cases at the P2P level (e.g.- in ProcessMessage). A wallet process may accept P2P block messages and do something with them, but that processing should be way different than what a node process does. If the wallet process was doing all the same work all over again, then the main benefits of the separation would be only run time process/network isolation and perhaps availability.

I'm assuming that the wallet-node design evolves so that the wallet process becomes heavily dependent on its trusted node process (which is ok) to determine the validity of and then pass on only valid and possibly filtered blocks and transactions. Why even check the header in the wallet process? Just take what is received from the trusted node, and update the TXO info. Having a designed goal for the wallet process would help giving feedback for this PR because, for example, it may be the case that the wallet only needs to maintain a header chain for re-org handling and then set (maybe bloom) filters in its trusted nodes to get what it needs for TXO management.

There could be lots of flag interactions like, for wallet processes, only connect to explicitly designated endpoints rather than trying to find seed nodes, etc... Assuming a shared code base, having some polymorphism in parameter interaction and P2P message processing could be valuable. But, I tend to think separate repo is preferable.

I think a good vision for how the wallet and node should interact would help review this PR. Then questions about code base structure, and whether to keep it all in the same binary or not can be answered.

rebroad commented at 1:03 PM on December 11, 2016: contributor

So is this a step towards being able to run bitcoind as a NETWORK_NODE and run bitcoin-qt as an SPV client connecting to it (and trusting it)?

jtimon commented at 4:03 AM on December 15, 2016: contributor

needs rebase

rebroad commented at 7:00 AM on December 30, 2016: contributor

What would be involved in getting with working with pruned-node ability also? And why not make it download all the headers so that it is at least connect to the genesis block as suggested by @dooglus ?

jonasschnelli commented at 1:10 PM on January 24, 2017: contributor

Closing in favour of #9483

jonasschnelli closed this on Jan 24, 2017

jonasschnelli removed this from the "Conceptual PR" column in a project

MarcoFalke locked this on Sep 8, 2021