Complete hybrid full block SPV mode #9483

jonasschnelli commented at 4:44 PM on January 6, 2017: contributor

This is the complete patch-set for the hybrid full block SPV mode.

If one enables the SPV mode with -spv=1 it does...

...first sync all headers (no block downloads during that phase)
...requests and persist all blocks that are relevant for the wallet (down to the dept of the older wallet key)
...scan the block for relevant transactions and flag them with validated = false (visible in listtransactions etc).
... continue with IBD (initial block download) after all wallet relevant blocks have been processed

Pure full block SPV mode is possible by setting -autorequestblocks=0, in that mode, no blocks for validating the chain will be downloaded, resulting in a SPV only mode.

For better testing, this PR also includes a bump to 0.0005 for the default fallback fee.

Including all required GUI changes and RPC tests:

Screenshots:

untitled-1

untitled-2

jonasschnelli added the label GUI on Jan 6, 2017

jonasschnelli added the label Wallet on Jan 6, 2017

luke-jr commented at 5:14 PM on January 6, 2017: member

(Prefer if we don't propagate the misuse of "SPV" for things that don't support fraud proofs)

gmaxwell commented at 9:24 PM on January 6, 2017: contributor

I kinda want to use an open lock icon instead of the likely meaningless to uses SPV in any case.

We also should do something about the confirmed counts in this mode. I'm not sure what. The issue is that confirmations mean less when you're not validating. Perhaps displaying transactions like they are unconfirmed until they have 6 blocks might be the thing to do. Or displaying a visible "not-verified" on any transaction with the not validated flag.

luke-jr commented at 11:42 PM on January 6, 2017: member

Indeed, I would assume any mode like this shouldn't count confirmation at all.

dabura667 commented at 5:05 AM on January 7, 2017: none

How about a flag for showing confirmations during SPV mode? Default to off.

People who understand the implications and just don't want to bother having to search their address on an explorer can enable in the menu / config

luke-jr commented at 5:52 AM on January 7, 2017: member

@dabura667 Is it sufficient to simply show it in the transaction details dialog, perhaps?

dabura667 commented at 9:13 AM on January 7, 2017: none

@luke-jr I would think so, yes.

molxyz commented at 6:58 PM on January 8, 2017: none

On transactions screen, fully-confirmed receiving txs show only one confirmation, and fully-confirmed sending txs show "unconfirmed" with question marks.
spv-txscreen

diegoviola commented at 7:45 PM on January 8, 2017: contributor

@molxyz I've also noticed the same thing, confirmations in Transactions list don't update, unless I restart Core.

Other than that it works great.

I used this for testing: ./bitcoin-qt -spv=1 -autorequestblocks=0 -testnet.

jonasschnelli commented at 9:35 PM on January 8, 2017: contributor

Thanks for reporting. This seems to be a UI update issue. Will fix it in the next overhaul / PR update.

luke-jr commented at 8:25 PM on January 20, 2017: member

Thought: Can this be made to work with external wallets/software?

jonasschnelli commented at 9:29 AM on January 21, 2017: contributor

Thought: Can this be made to work with external wallets/software?

I don't know what you mean by this. Can you make an use-case example?

jtimon commented at 2:12 PM on January 24, 2017: contributor

Needs rebase. Concept ACK

What happens with -autorequestblocks=0 -spv=0? I assume both -spv and -autorequestblocks are 1 by default. With -spv=0 -autorequestblocks=1 you would get what you have today, but is it really so useful compared to -spv=1 -autorequestblocks=1 ?

I don't know it seems overly complicated. I thought we would just have a single param spvonly that defaults to 0 (equivalent to this autorequestblocks, and your spv is always =1, ie spvonly=0 equivalent to -spv=1 -autorequestblocks=1, spvonly=1 equivalent to -spv=1 -autorequestblocks=0). Not sure, just thinking out loud.

jonasschnelli force-pushed on Jan 24, 2017

Add CAuxiliaryBlockRequest, a class to handle auxiliary blocks downloads

This is required for features that need to download and process block higher/further-away then the current validation depth

3a6364de88

Pass CBlockRequest blocks through SyncTransaction signal

+ Adds a validate=true|false to the SyncTransaction signal

340e363a7f

CBlockRequest: make SyncTransaction() optional ba99197aef

Add requestblocks - request out-of-band blocks download - RPC call df6e5b873e

[Wallet] don't consume non-validated transactions 32770b8bf7

Add -autorequestblocks debug option and setautorequestblocks hidden RPC call

This allows efficient testing of auxiliary block requests

567055e6c4

[QA] Add auxiliary block request test b7ef93d765

Allow CheckFinalTx() without validation using the headers chain a86c66d9da

Add FindTransaction signal: allows to inv transactions that are not in the mempool 7ca1a8738a

Add CChain object for headers-only chain 1687a0e5bc

Track the validation state of a transaction (CMerkleTx::fValidated) 4cbaf6c0e1

jonasschnelli force-pushed on Jan 24, 2017

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

Rebased.

What happens with -autorequestblocks=0 -spv=0?

This would result in a mode where no blocks are automatically requested (only headers are fetched). autorequestblocks=0 is a debug option and I could imagine some interesting use-cases where you only want to fetch certain blocks with requestblocks RPC call.

Add basic non-validation mode to the wallet 1cc9293bfa

Add UpdateBlockHeaderTip signal cfe9921421

Keep track of the headers chain tip to detect forks 1d3011ef91

Little CAuxiliaryBlockRequest refactor 48c4c3c7c1

Add full working SPV mode to the wallet 5585662d37

[Qt] add basic SPV support for the transaction table 5826a5de17

[Qt] Add status bar icon to indicate SPV mode e783bae272

Add RPC call to enabled/disabled SPV mode 3756ad3735

[Qt] Show auxiliary-block-request/SPV progress in the UI cb1ff093c3

Bump default tx fee from 0.0002 to 0.0005 btc/kb 3f7aed91ce

[Qt] Avoid in-between animation state of ModalOverlay 346a917963

[Qt] Hide ModalOverlay by default when SPV is enabled 749b937ac6

[Qt] Show more significant warning if we fall back to the default fee 87fb7fbcb0

Don't fetch blocks during headers-sync when SPV is enabled 9c4055dc46

[Qt] Update the transaction table based on changes of the header-chain 254c94ca45

jonasschnelli force-pushed on Jan 24, 2017

jonasschnelli commented at 8:12 PM on January 24, 2017: contributor

Adapted to work with bumpfee. Added a fix for the UI update issue reported by @molxyz and @diegoviola (https://github.com/bitcoin/bitcoin/pull/9483#issuecomment-271170997).

in src/wallet/walletdb.h:None in 254c94ca45

 140 | @@ -141,6 +141,9 @@ class CWalletDB : public CDB
 141 |      bool WriteBestBlock(const CBlockLocator& locator);
 142 |      bool ReadBestBlock(CBlockLocator& locator);
 143 |  
 144 | +    bool WriteNonValidationBestBlock(const CBlockLocator& locator);
 145 | +    bool ReadNonValidationBestBlock(CBlockLocator& locator);

ryanofsky commented at 10:43 PM on January 24, 2017:

It would be nice if you could choose one of the terms "nonvalidation" "nonvalidated" and "nvs" and use it consistently everywhere in the code (personally I like "nonvalidated"). Right now it seems like everything is named randomly and it's hard to remember what things are called.

in src/net_processing.cpp:None in 254c94ca45

3121 | @@ -3085,13 +3122,16 @@ bool SendMessages(CNode* pto, CConnman& connman, std::atomic<bool>& interruptMsg
3122 |                      }
3123 |                      // Not in the mempool anymore? don't bother sending it.

ryanofsky commented at 10:50 PM on January 24, 2017:

Comment is out of date. Could you update it to mention that this now checks the wallet, and also mention why it does this (maybe it should be obvious, but the reason isn't clear to me yet).

in src/wallet/wallet.cpp:None in 254c94ca45

1211 | @@ -1182,6 +1212,14 @@ void CWallet::SyncTransaction(const CTransaction& tx, const CBlockIndex *pindex,
1212 |      }
1213 |  }
1214 |  
1215 | +void CWallet::GetNonMempoolTransaction(const uint256 &hash, CTransactionRef &txsp)

ryanofsky commented at 10:54 PM on January 24, 2017:

Any reason not to call this FindTransaction like the signal which it binds to? FindTransaction definitely seems like a better name given the implementation because if this method is called, it will return a transaction whether it is in the mempool or not.

in src/wallet/wallet.cpp:None in 254c94ca45

1211 | @@ -1182,6 +1212,14 @@ void CWallet::SyncTransaction(const CTransaction& tx, const CBlockIndex *pindex,
1212 |      }
1213 |  }
1214 |  
1215 | +void CWallet::GetNonMempoolTransaction(const uint256 &hash, CTransactionRef &txsp)
1216 | +{
1217 | +    map<uint256, CWalletTx>::const_iterator mi = mapWallet.find(hash);
1218 | +    if (mi != mapWallet.end())
1219 | +    {
1220 | +        txsp = MakeTransactionRef(mi->second);

ryanofsky commented at 10:55 PM on January 24, 2017:

txsp = mi->second.tx; might be better to avoid a temporary.

in src/wallet/wallet.cpp:None in 254c94ca45

1184 | +    if (pNVSLastKnownBestHeader && !headersChainActive.Contains(pNVSLastKnownBestHeader))
1185 | +    {
1186 | +        const CBlockIndex *pindexFork = headersChainActive.FindFork(pNVSLastKnownBestHeader);
1187 | +        if (headersChainActive.Tip() && headersChainActive.Tip() != pindexFork)
1188 | +        {
1189 | +            pNVSLastKnownBestHeader = const_cast<CBlockIndex *>(pindexFork);

ryanofsky commented at 11:03 PM on January 24, 2017:

I think you can just make pNVSLastKnownBestHeader and pNVSBestBlock into const pointers to avoid these const_casts.

in src/wallet/wallet.cpp:None in 254c94ca45

1174 | @@ -1165,11 +1175,31 @@ void CWallet::MarkConflicted(const uint256& hashBlock, const uint256& hashTx)
1175 |      }
1176 |  }
1177 |  
1178 | -void CWallet::SyncTransaction(const CTransaction& tx, const CBlockIndex *pindex, int posInBlock)
1179 | +void CWallet::UpdatedBlockHeaderTip(bool fInitialDownload, const CBlockIndex *pindexNew)

ryanofsky commented at 11:05 PM on January 24, 2017:

fInitialDownload isn't actually used, though maybe it is worth keeping if you plan to unify with the NotifyHeaderTip signal in the future.

ryanofsky commented at 11:12 PM on January 24, 2017: member

Just started reviewing this. Plan to review more this week but posting a few comments now, all minor.

in src/auxiliaryblockrequest.h:None in 3a6364de88 outdated

  11 | +#include "net.h"
  12 | +#include <stdint.h>
  13 | +#include <vector>
  14 | +
  15 | +// "Lock free" auxiliary block request
  16 | +class CAuxiliaryBlockRequest : public std::enable_shared_from_this<CAuxiliaryBlockRequest> {

ryanofsky commented at 2:34 PM on February 8, 2017:

I think it would be better if this class didn't exist, and the logic to download and process these blocks was integrated into the normal network processing logic instead of being segregated. This could mean:

Getting rid of the new currentBlockRequest global variable. Instead just add a simple deque<const CBlockIndex*> blocksToDownloadFirst or similar variable in net_processing.cpp alongside related variables like mapBlocksInFlight.
Replacing CAuxiliaryBlockRequest::progressCallback with an ordinary validationinterface signal.
Moving logic currently in CAuxiliaryBlockRequest::fillInNextBlocks to FindNextBlocksToDownload. Instead of having a convoluted control flow where FindNextBlocksToDownload calls fillInNextBlocks which calls back to a FindNextBlocksToDownload closure just to push a few CBlockIndex pointers onto a vector, you could use a regular for loop to fill the vector.
Moving logic currently in CAuxiliaryBlockRequest::processWithPossibleBlock into ProcessNewBlock or into an analog of ActivateBestChain called by ProcessNewBlock that sends the needed wallet and ui notifications for unvalidated blocks in the same way that ActivateBestChain does for validated blocks.

in src/validation.h:None in 3a6364de88 outdated

 231 | @@ -231,7 +232,7 @@ static const uint64_t MIN_DISK_SPACE_FOR_BLOCK_FILES = 550 * 1024 * 1024;
 232 |   * @param[out]  fNewBlock A boolean which is set to indicate if the block was first received via this call

ryanofsky commented at 4:02 PM on February 8, 2017:

Doxygen comment is now out of date.

in src/validation.cpp:None in 3a6364de88 outdated

3199 | +    else
3200 | +    {
3201 | +        // Try to process all requested blocks that we don't have, but only
3202 | +        // process an unrequested block if it's new and has enough work to
3203 | +        // advance our tip, and isn't too many blocks ahead.
3204 | +        bool fAlreadyHave = pindex->nStatus & BLOCK_HAVE_DATA;

ryanofsky commented at 4:22 PM on February 8, 2017:

Do you really want to skip this whole section just because blockRequest is not null? E.g. wouldn't it make sense to avoid WriteBlockToDisk below when fAlreadyHave is true like this is doing?

Also it's not clear to me whether BLOCK_FAILED_VALID and setDirtyBlockIndex below should be updated for blockRequest blocks. If you know that they should not be updated, it would be useful to have a comment explaining why.

in src/net_processing.h:None in 340e363a7f outdated

  29 | @@ -30,7 +30,7 @@ class PeerLogicValidation : public CValidationInterface {
  30 |  public:
  31 |      PeerLogicValidation(CConnman* connmanIn);
  32 |  
  33 | -    virtual void SyncTransaction(const CTransaction& tx, const CBlockIndex* pindex, int nPosInBlock);
  34 | +    virtual void SyncTransaction(const CTransaction& tx, const CBlockIndex* pindex, int nPosInBlock, bool validated);

ryanofsky commented at 6:31 PM on February 8, 2017:

Why change signature of SyncTransaction when the value of the new argument can already be derived from pindex? It seems like it would make handling code clearer if the validated condition were written out there where it is actually used, instead of determined by the networking code and then passed along.

jonasschnelli commented at 10:42 AM on July 11, 2017:

Good point.

in src/rpc/blockchain.cpp:None in df6e5b873e outdated

1462 | +        std::shared_ptr<CAuxiliaryBlockRequest> blockRequest = CAuxiliaryBlockRequest::GetCurrentRequest();
1463 | +        UniValue ret(UniValue::VOBJ);
1464 | +        ret.pushKV("request_present", (bool)blockRequest);
1465 | +        if (blockRequest) {
1466 | +            ret.pushKV("created", UniValue(blockRequest->created));
1467 | +            ret.pushKV("is_cancled", UniValue(blockRequest->isCancelled()));

ryanofsky commented at 6:38 PM on February 8, 2017:

Maybe change "is_cancled" to "cancelled" (to be consistent with "created"). Spelling error is also above in rpc documentation.

in src/rpc/blockchain.cpp:None in df6e5b873e outdated

1418 | @@ -1413,6 +1419,98 @@ UniValue reconsiderblock(const JSONRPCRequest& request)
1419 |      return NullUniValue;
1420 |  }
1421 |  
1422 | +UniValue requestblocks(const JSONRPCRequest& request)

ryanofsky commented at 6:58 PM on February 8, 2017:

Can you explain what the the use-cases for this api are, and also what the use cases for the cancellation and SyncTransaction-suppressing options are? It doesn't seem good that this RPC can interfere with block download in spv mode and prevent transactions from getting to the wallet, but I can't figure out if this is designed to interact this way intentionally or if it is something that should be fixed.

jonasschnelli commented at 12:21 PM on July 11, 2017:

The use cases for the RPC requestblocks API: You start your peer with auto-download-blocks = false, you will only sync the headers then. You can selectively download blocks and eventually pass them through the signal (== ZMQ), use cases: experiments, SPV, light-client backend, ideally if you have a full validated node within your network and you want to selectively load blocks from that node (you don't want to validate everything again).

in src/rpc/blockchain.cpp:None in df6e5b873e outdated

1480 | +            throw JSONRPCError(RPC_INVALID_PARAMETER, "Second parameter must be an array");
1481 | +
1482 | +        std::vector<const CBlockIndex*> blocksToDownload;
1483 | +        {
1484 | +            LOCK(cs_main); //mapBlockIndex
1485 | +            for (UniValue strHashU : hash_Uarray.getValues())

ryanofsky commented at 6:59 PM on February 8, 2017:

Maybe avoid copy with const &

in src/rpc/blockchain.cpp:None in df6e5b873e outdated

1530 | @@ -1433,8 +1531,8 @@ static const CRPCCommand commands[] =
1531 |      { "blockchain",         "gettxoutsetinfo",        &gettxoutsetinfo,        true,  {} },
1532 |      { "blockchain",         "pruneblockchain",        &pruneblockchain,        true,  {"height"} },
1533 |      { "blockchain",         "verifychain",            &verifychain,            true,  {"checklevel","nblocks"} },
1534 | -
1535 |      { "blockchain",         "preciousblock",          &preciousblock,          true,  {"blockhash"} },
1536 | +    { "blockchain",         "requestblocks",          &requestblocks,          true,  {"action", "blockhashes", "pass-internally"} },

ryanofsky commented at 7:12 PM on February 8, 2017:

Probably should use "pass_internally" (underscore instead of dash) so the argument can be a valid identifier in python and other languages.

in src/rpc/blockchain.cpp:None in 1687a0e5bc outdated

1251 | @@ -1251,9 +1252,12 @@ UniValue getchaintips(const JSONRPCRequest& request)
1252 |          } else if (block->nStatus & BLOCK_FAILED_MASK) {
1253 |              // This block or one of its ancestors is invalid.
1254 |              status = "invalid";
1255 | -        } else if (block->nChainTx == 0) {
1256 | +        } else if (headersChainActive.Contains(block)) {

ryanofsky commented at 9:11 PM on February 8, 2017:

Seems like it makes sense to return "headers-only-fork" when block->nChainTx == 0 && !headersChainActive.Contains(block), but I don't see how it makes sense to return "headers-only" when block->nChainTx != 0 && headersChainActive.Contains(block) instead of valid-fork or valid-headers when those conditions apply.

Maybe this should be changed to:

} else if (block->nChainTx == 0) {
    // This block cannot be connected because full block data for it or one of its parents is missing.
    status = headersChainActive.Contains(block) ? "headers-only-fork" : "headers-only";
} ...

in src/wallet/wallet.cpp:None in 1cc9293bfa outdated

1943 | @@ -1947,7 +1944,7 @@ CAmount CWallet::GetUnconfirmedBalance() const
1944 |          for (map<uint256, CWalletTx>::const_iterator it = mapWallet.begin(); it != mapWallet.end(); ++it)
1945 |          {
1946 |              const CWalletTx* pcoin = &(*it).second;
1947 | -            if (!pcoin->IsTrusted() && pcoin->GetDepthInMainChain() == 0 && pcoin->InMempool())
1948 | +            if (!pcoin->IsTrusted() && pcoin->GetDepthInMainChain() == 0 && (!pcoin->fValidated || pcoin->InMempool()))

ryanofsky commented at 9:55 PM on February 8, 2017:

!pcoin->fValidated seems like it might be too lax a condition, because the transaction could be from an orphaned block.

And it's unclear why this change would have any desirable effect given that GetDepthInMainChain is updated in this commit to search headersChainActive, so presumably any nonvalidated transaction in headersChainActive would return true for IsTrusted.

in src/wallet/wallet.cpp:None in 1d3011ef91 outdated

1181 | +        {
1182 | +            const CBlockIndex *pindexFork = headersChainActive.FindFork(pLastKnownBestHeader);
1183 | +            if (headersChainActive.Tip() && headersChainActive.Tip() != pindexFork)
1184 | +            {
1185 | +                // fork detected
1186 | +                // TODO

ryanofsky commented at 10:06 PM on February 8, 2017:

Probably should just squash this commit (Keep track of the headers chain tip to detect forks) together with commit Add full working SPV mode to the wallet to avoid the code churn here.

in src/wallet/wallet.cpp:None in 1d3011ef91 outdated

1185 | +                // fork detected
1186 | +                // TODO
1187 | +            }
1188 | +        }
1189 | +    }
1190 | +    pLastKnownBestHeader = (CBlockIndex *)pindexNew;

ryanofsky commented at 10:07 PM on February 8, 2017:

Should just make pLastKnownBestHeader a pointer to const to avoid the need for this cast.

in src/wallet/wallet.cpp:None in 5585662d37 outdated

3913 | @@ -3897,6 +3914,95 @@ bool CWallet::ParameterInteraction()
3914 |      return true;
3915 |  }
3916 |  
3917 | +void CWallet::RequestNonValidationScan(int64_t optional_timestamp)
3918 | +{
3919 | +    if (CAuxiliaryBlockRequest::GetCurrentRequest() && !CAuxiliaryBlockRequest::GetCurrentRequest()->isCompleted())

ryanofsky commented at 10:11 PM on February 8, 2017:

There's a race condition here where GetCurrentRequest() can return null between the first and second calls.

in src/wallet/wallet.cpp:None in 5585662d37 outdated

3919 | +    if (CAuxiliaryBlockRequest::GetCurrentRequest() && !CAuxiliaryBlockRequest::GetCurrentRequest()->isCompleted())
3920 | +        return;
3921 | +
3922 | +    CBlockIndex *pIndex = NULL;
3923 | +    CBlockIndex *chainActiveTip = NULL;
3924 | +    int64_t oldest_key = std::numeric_limits<int64_t>::max();;

ryanofsky commented at 10:12 PM on February 8, 2017:

double semicolon here, also probably should choose consistently between snake_case and camelCase for local variable names.

in src/wallet/wallet.cpp:None in 5585662d37 outdated

3929 | +            nonValidationScanUpToHeight = pNVSBestBlock->nHeight;
3930 | +        chainActiveTip = chainActive.Tip();
3931 | +        pIndex = headersChainActive.Tip();
3932 | +        std::map<CKeyID, int64_t> mapKeyBirth;
3933 | +        GetKeyBirthTimes(mapKeyBirth);
3934 | +        for (std::map<CKeyID, int64_t>::const_iterator it = mapKeyBirth.begin(); it != mapKeyBirth.end(); it++) {

ryanofsky commented at 10:13 PM on February 8, 2017:

Maybe for (const auto& entry : mapKeyBirth)

in src/wallet/wallet.cpp:None in 5585662d37 outdated

3930 | +        chainActiveTip = chainActive.Tip();
3931 | +        pIndex = headersChainActive.Tip();
3932 | +        std::map<CKeyID, int64_t> mapKeyBirth;
3933 | +        GetKeyBirthTimes(mapKeyBirth);
3934 | +        for (std::map<CKeyID, int64_t>::const_iterator it = mapKeyBirth.begin(); it != mapKeyBirth.end(); it++) {
3935 | +            if ((*it).second < oldest_key)

ryanofsky commented at 10:14 PM on February 8, 2017:

Maybe it->second

in src/wallet/wallet.cpp:None in 5585662d37 outdated

3913 | @@ -3897,6 +3914,95 @@ bool CWallet::ParameterInteraction()
3914 |      return true;
3915 |  }
3916 |  
3917 | +void CWallet::RequestNonValidationScan(int64_t optional_timestamp)

ryanofsky commented at 10:23 PM on February 8, 2017:

Eliminate optional_timestamp argument? I don't see any calls where optional_timestamp is nonzero.

in src/wallet/wallet.cpp:None in 5585662d37 outdated

3935 | +            if ((*it).second < oldest_key)
3936 | +                oldest_key = (*it).second;
3937 | +        }
3938 | +    }
3939 | +
3940 | +    if (optional_timestamp > 0)

ryanofsky commented at 10:24 PM on February 8, 2017:

As noted above, this case never seems to happen. But if it could happen, it would seem to negate all the code right above this, so maybe that code should be moved into an else.

in src/wallet/wallet.cpp:None in 5585662d37 outdated

3979 | +
3980 | +        LOCK2(cs_main, cs_wallet);
3981 | +        if (pindex && (!pNVSBestBlock || pindex->nHeight > pNVSBestBlock->nHeight))
3982 | +        {
3983 | +            // write non validation best block
3984 | +            pNVSBestBlock = const_cast<CBlockIndex *>(pindex);

ryanofsky commented at 10:30 PM on February 8, 2017:

No need for const_cast if you just make pNVSBestBlock a const pointer.

in src/wallet/wallet.cpp:None in 5585662d37 outdated

3963 | +        // block is relevant, request
3964 | +        blocksToDownload.push_back(pIndex);
3965 | +
3966 | +        // ensure we only request up to nMaxBlocksPerAuxiliaryRequest
3967 | +        if (blocksToDownload.size() > nMaxBlocksPerAuxiliaryRequest)
3968 | +            blocksToDownload.erase(blocksToDownload.begin());

ryanofsky commented at 10:34 PM on February 8, 2017:

Could switch to from vector to deque, or treat blocksToDownload as a circular buffer and call std::rotate after the loop to avoid O(n^2) cost of erasing from the beginning of a vector in a loop.

in src/wallet/rpcwallet.cpp:None in 3756ad3735 outdated

2913 | @@ -2913,6 +2914,32 @@ UniValue bumpfee(const JSONRPCRequest& request)
2914 |      return result;
2915 |  }
2916 |  
2917 | +UniValue setspv(const JSONRPCRequest& request)
2918 | +{
2919 | +    if (!EnsureWalletIsAvailable(request.fHelp))
2920 | +        return NullUniValue;
2921 | +
2922 | +    if (request.fHelp || request.params.size() > 1)

ryanofsky commented at 3:04 PM on February 9, 2017:

Maybe should throw if no arguments passed.

in src/wallet/rpcwallet.cpp:None in 3756ad3735 outdated

2931 | +                            + HelpExampleCli("setspv", "\"true\"")
2932 | +                            + HelpExampleRpc("setspv", "\"true\"")
2933 | +                            );
2934 | +
2935 | +    if (request.params.size() == 1)
2936 | +        pwalletMain->setSPVEnabled(request.params[0].get_bool());

ryanofsky commented at 3:08 PM on February 9, 2017:

Seems incongruous that unlike the setting -spv option, enabling spv via RPC does not update fFetchBlocksWhileFetchingHeaders

in src/qt/walletmodel.cpp:None in e783bae272 outdated

 715 | +    return wallet->IsSPVEnabled();
 716 | +}
 717 | +
 718 | +void WalletModel::setSpvEnabled(bool state)
 719 | +{
 720 | +    wallet->setSPVEnabled(state);

ryanofsky commented at 3:11 PM on February 9, 2017:

Seems incongruous that unlike setting the -spv option, toggling the spv mode in the GUI does not update fFetchBlocksWhileFetchingHeaders.

in src/net_processing.cpp:None in 9c4055dc46 outdated

3187 | @@ -3187,7 +3188,9 @@ bool SendMessages(CNode* pto, CConnman& connman, std::atomic<bool>& interruptMsg
3188 |          // Message: getdata (blocks)
3189 |          //
3190 |          vector<CInv> vGetData;
3191 | -        if (!pto->fClient && (fFetch || !IsInitialBlockDownload()) && state.nBlocksInFlight < MAX_BLOCKS_IN_TRANSIT_PER_PEER) {
3192 | +        if (!pto->fClient && (fFetch || !IsInitialBlockDownload()) && state.nBlocksInFlight < MAX_BLOCKS_IN_TRANSIT_PER_PEER
3193 | +                && (fFetchBlocksWhileFetchingHeaders || (headersChainActive.Tip() && headersChainActive.Tip()->GetBlockTime() > GetAdjustedTime()-600*24))

ryanofsky commented at 4:59 PM on February 9, 2017:

I think it would make the logic of this PR (and the code) clearer if both fFetchBlocksWhileFetchingHeaders and fAutoRequestBlocks flags were both handled inside of FindNextBlocksToDownload, instead of one flag handled inside, and one outside.

ryanofsky commented at 5:25 PM on February 9, 2017:

Should 600 be 3600?

in src/net_processing.h:None in 9c4055dc46 outdated

  26 | @@ -27,6 +27,9 @@ void UnregisterNodeSignals(CNodeSignals& nodeSignals);
  27 |  static const bool DEFAULT_AUTOMATIC_BLOCK_REQUESTS = true;
  28 |  extern std::atomic<bool> fAutoRequestBlocks;
  29 |  
  30 | +static const bool DEFAULT_FETCH_BLOCKS_WHILE_FETCH_HEADERS = true;
  31 | +extern std::atomic<bool> fFetchBlocksWhileFetchingHeaders;

ryanofsky commented at 5:24 PM on February 9, 2017:

Maybe rename fFetchBlocksWhileFetchingHeaders to fRequestBlocksWhileFetchingHeaders for consistency with fAutoRequestBlocks.

in src/init.cpp:None in 567055e6c4 outdated

 434 | @@ -435,6 +435,7 @@ std::string HelpMessage(HelpMessageMode mode)
 435 |          strUsage += HelpMessageOpt("-limitdescendantcount=<n>", strprintf("Do not accept transactions if any ancestor would have <n> or more in-mempool descendants (default: %u)", DEFAULT_DESCENDANT_LIMIT));
 436 |          strUsage += HelpMessageOpt("-limitdescendantsize=<n>", strprintf("Do not accept transactions if any ancestor would have more than <n> kilobytes of in-mempool descendants (default: %u).", DEFAULT_DESCENDANT_SIZE_LIMIT));
 437 |          strUsage += HelpMessageOpt("-bip9params=deployment:start:end", "Use given start/end times for specified BIP9 deployment (regtest-only)");
 438 | +        strUsage += HelpMessageOpt("-autorequestblocks", strprintf("Automatic block request, if disabled, blocks will not be requested in IBD/sync-up (default: %u)", DEFAULT_AUTOMATIC_BLOCK_REQUESTS));

ryanofsky commented at 7:33 PM on February 9, 2017:

Need to update the man page, too, I believe. Maybe also worth documenting that when -autorequestblocks is disabled, it only prevents blocks downloaded as part of the normal sync. It doesn't prevent downloading of blocks newer than the oldest wallet key in the -spv syncing code.

in src/net_processing.cpp:None in 567055e6c4 outdated

 508 | @@ -507,6 +509,10 @@ void FindNextBlocksToDownload(NodeId nodeid, unsigned int count, std::vector<con
 509 |          return;
 510 |      }
 511 |  
 512 | +    // don't request any other blocks if we are in non autorequest mode (usefull for non-validation mode)

ryanofsky commented at 7:37 PM on February 9, 2017:

spelling "useful"

in src/wallet/wallet.cpp:None in 5585662d37 outdated

1189 | -            if (headersChainActive.Tip() && headersChainActive.Tip() != pindexFork)
1190 | -            {
1191 | -                // fork detected
1192 | -                // TODO
1193 | -            }
1194 | +            pNVSLastKnownBestHeader = const_cast<CBlockIndex *>(pindexFork);

ryanofsky commented at 7:57 PM on February 9, 2017:

Const casts here should not be necessary if you declare pNVSLastKnownBestHeader and pNVSLastKnownBestHeader as pointers to const CBlockIndex objects.

in src/wallet/rpcwallet.cpp:None in 1cc9293bfa outdated

1151 | @@ -1151,7 +1152,7 @@ UniValue ListReceived(const UniValue& params, bool fByAccounts)
1152 |      {
1153 |          const CWalletTx& wtx = (*it).second;
1154 |  
1155 | -        if (wtx.IsCoinBase() || !CheckFinalTx(*wtx.tx))
1156 | +        if (wtx.IsCoinBase() || !CheckFinalTx(*wtx.tx, -1, wtx.fValidated))

ryanofsky commented at 8:03 PM on February 9, 2017:

Seems to be a bug, this should probably say !wtx.fValidated.

This CheckFinalTx(*wtx.tx, -1, !wtx.fValidated) pattern is repeated enough times that I think it would be better if it were wrapped in a method:

bool CWalletTx::CheckFinal() const {
    return CheckFinalTx(*tx, -1, !fValidated);
}

in src/wallet/wallet.cpp:None in 5585662d37 outdated

3973 | +    if (blocksToDownload.size() == 0)
3974 | +        return;
3975 | +    // reverse the blocks vector from older->newer
3976 | +    std::reverse(blocksToDownload.begin(), blocksToDownload.end());
3977 | +    // create an auxiliary block request
3978 | +    std::shared_ptr<CAuxiliaryBlockRequest> auxiliaryRequest(new CAuxiliaryBlockRequest(blocksToDownload, GetAdjustedTime(), true, [this](std::shared_ptr<CAuxiliaryBlockRequest> cb_AuxiliaryBlockRequest, const CBlockIndex *pindex) -> bool {

ryanofsky commented at 8:08 PM on February 9, 2017:

Maybe use make_shared.

in src/rpc/blockchain.cpp:None in df6e5b873e outdated

1497 | +            passThroughSignals = request.params[2].get_bool();
1498 | +
1499 | +        std::shared_ptr<CAuxiliaryBlockRequest> blockRequest(new CAuxiliaryBlockRequest(blocksToDownload, GetAdjustedTime(), passThroughSignals, [](std::shared_ptr<CAuxiliaryBlockRequest> cb_spvRequest, const CBlockIndex *pindex) -> bool {
1500 | +            return true;
1501 | +        }));
1502 | +        bool overwrite = (CAuxiliaryBlockRequest::GetCurrentRequest() != nullptr);

ryanofsky commented at 8:17 PM on February 9, 2017:

There's a race condition here if the state changes between the GetCurrentRequest() and setAsCurrentRequest() calls. Could easily be avoided by having setAsCurrentRequest return the overwrite bool, or a pointer to the previous request.

in src/rpc/blockchain.cpp:None in df6e5b873e outdated

1494 | +
1495 | +        bool passThroughSignals = false;
1496 | +        if (request.params.size() == 3 && request.params[2].isBool())
1497 | +            passThroughSignals = request.params[2].get_bool();
1498 | +
1499 | +        std::shared_ptr<CAuxiliaryBlockRequest> blockRequest(new CAuxiliaryBlockRequest(blocksToDownload, GetAdjustedTime(), passThroughSignals, [](std::shared_ptr<CAuxiliaryBlockRequest> cb_spvRequest, const CBlockIndex *pindex) -> bool {

ryanofsky commented at 8:20 PM on February 9, 2017:

Could use make_shared.

ryanofsky commented at 8:28 PM on February 9, 2017:

Could just pass {} or nullptr for progress function instead of writing out the long lambda declaration.

jonasschnelli commented at 2:58 PM on July 11, 2017:

Would returning true be guaranteed then?

in src/validation.cpp:None in a86c66d9da outdated

 257 | +    //
 258 | +    // If calcHeightFromHeaders is set to true, we use the headers-
 259 | +    // chain-tip to calculate the block height
 260 | +    CChain *chainToUse = &chainActive;
 261 | +    if (calcHeightFromHeaders && headersChainActive.Tip())
 262 | +        chainToUse = &headersChainActive;

ryanofsky commented at 8:41 PM on February 9, 2017:

headersChainActive variable doesn't seem to be defined yet. Should reorder this commit ("Allow CheckFinalTx() without validation using the headers chain") after "Add CChain object for headers-only chain."

in src/auxiliaryblockrequest.h:None in ba99197aef outdated

  19 | @@ -20,9 +20,10 @@ class CAuxiliaryBlockRequest : public std::enable_shared_from_this<CAuxiliaryBlo
  20 |  
  21 |      const std::vector<const CBlockIndex*> vBlocksToDownload;
  22 |      const int64_t created; //!timestamp when the block request was created
  23 | +    const bool passThroughSignals; //!if passThroughSignals is set, the received blocks transaction will be passed through the SyncTransaction signal */
  24 |  
  25 |      /** Constructor of the lock free CAuxiliaryBlockRequest, vBlocksToDownloadIn remains constant */
  26 | -    CAuxiliaryBlockRequest(std::vector<const CBlockIndex*> vBlocksToDownloadIn, int64_t created, const std::function<bool(std::shared_ptr<CAuxiliaryBlockRequest>, const CBlockIndex *pindex)> progressCallbackIn);
  27 | +    CAuxiliaryBlockRequest(std::vector<const CBlockIndex*> vBlocksToDownloadIn, int64_t created, bool passThroughSignalsIn, const std::function<bool(std::shared_ptr<CAuxiliaryBlockRequest>, const CBlockIndex *pindex)> progressCallbackIn);

ryanofsky commented at 8:52 PM on February 9, 2017:

Probably should just squash this "CBlockRequest: make SyncTransaction() optional" commit into "Add CAuxiliaryBlockRequest, a class to handle auxiliary blocks downloads" to avoid churn.

in src/wallet/wallet.cpp:None in 32770b8bf7 outdated

1168 | @@ -1169,6 +1169,9 @@ void CWallet::SyncTransaction(const CTransaction& tx, const CBlockIndex *pindex,
1169 |  {
1170 |      LOCK2(cs_main, cs_wallet);
1171 |  
1172 | +    if (!validated)

ryanofsky commented at 8:58 PM on February 9, 2017:

Would be good to squash this "[Wallet] don't consume non-validated transactions" commit into the "Pass CBlockRequest blocks through SyncTransaction signal" to make commit order less fragile and review more straightforward.

in src/rpc/blockchain.cpp:None in 567055e6c4 outdated

1511 | @@ -1511,6 +1512,30 @@ UniValue requestblocks(const JSONRPCRequest& request)
1512 |          throw JSONRPCError(RPC_INVALID_PARAMETER, "Unkown action");
1513 |  }
1514 |  
1515 | +UniValue setautorequestblocks(const JSONRPCRequest& request)
1516 | +{
1517 | +    if (request.fHelp || request.params.size() > 1)

ryanofsky commented at 8:59 PM on February 9, 2017:

Probably should throw if no arguments passed.

in src/wallet/wallet.cpp:None in 7ca1a8738a outdated

1184 | @@ -1185,6 +1185,14 @@ void CWallet::SyncTransaction(const CTransaction& tx, const CBlockIndex *pindex,
1185 |      }
1186 |  }
1187 |  
1188 | +void CWallet::GetNonMempoolTransaction(const uint256 &hash, CTransactionRef &txsp)

ryanofsky commented at 9:05 PM on February 9, 2017:

Method looks like it could be const.

in src/auxiliaryblockrequest.h:None in 48c4c3c7c1 outdated

  44 | @@ -45,6 +45,9 @@ class CAuxiliaryBlockRequest : public std::enable_shared_from_this<CAuxiliaryBlo
  45 |      /** returns the amount of already loaded/local-stored blocks from this blockrequest */
  46 |      unsigned int amountOfBlocksLoaded();
  47 |  
  48 | +    /** returns true if all blocks have been downloaded & processed */
  49 | +    bool isCompleted();

ryanofsky commented at 9:28 PM on February 9, 2017:

Should probably squash this commit (Little CAuxiliaryBlockRequest refactor) into commit Add CAuxiliaryBlockRequest, a class to handle auxiliary blocks downloads to avoid code churn in the review.

in src/qt/forms/sendcoinsdialog.ui:None in 87fb7fbcb0 outdated

 759 | @@ -760,10 +760,30 @@
 760 |             </layout>
 761 |            </item>
 762 |            <item>
 763 | +           <widget class="QLabel" name="fallbackFeeWarningLabel">
 764 | +            <property name="toolTip">
 765 | +             <string>Using the fallbackfee can result in sending a transaction that will take serval hours or days (or never) to confirm. Consider choosing your fee manually or wait until your have validated the complete chain.</string>

ryanofsky commented at 9:37 PM on February 9, 2017:

spelling: several

ryanofsky commented at 10:47 PM on February 9, 2017: member

I reviewed all the commits and left many minor comments, but I have two broader concerns about this PR that may be worth some discussion:

Architecture. It seems this change would be a lot simpler if it just added a deque<const CBlockIndex*> blocksToDownloadFirst net_processing variable that the wallet could add blocks to (through a function) and that net_processing code would process within its existing control flow (with some tweaks to FindNextBlocksToDownload and ProcessNewBlock). Aside from the simplifications this would allow, I also think this would be better because it would no longer be adding wallet code that has to be responsible for batching and orchestrating block downloads (in CWallet::RequestSPVScan).
Naming and UI. The -autorequestblocks flag makes sense to me. The -spv option makes less sense to me and I think it would be better off called something like -priorityrequestblocks. Renaming -spv to -priorityrequestblocks would give a more coherent set of options:

-priorityrequestblocks

       Prioritized block request. If enabled, full IBD/sync-up will be delayed
       until complete blockchain headers and the contents of blocks newer than
       the oldest wallet key have been downloaded. Any wallet transactions that
       are picked up in the prioritized blocks will show up as non-validated until
       the full IBD/sync completes. (default: 0)

-autorequestblocks

       Automatic block request. If disabled, no blocks will be requested in
       IBD/sync-up, and only previously downloaded blocks, and blocks 
       requested through the `requestblocks` RPC or `-priorityrequestblocks`
       option will be available. (default: 1)

Similarly, I don't think it makes sense to have a so-called "SPV mode" in the wallet that just reflects the -priorityrequestblocks setting while ignoring the -autorequestblocks setting. Certainly if there are non-validated transactions in the wallet, they should clearly show up as non-validated. And if the wallet transaction history in the wallet is incomplete, this should clearly be indicated (as discussed in #9409). But beyond these two things, I don't understand what the wallet "SPV mode" is supposed to indicate. Why would it be helpful to me to know that my wallet is in SPV mode if I don't actually have any nonvalidated transactions? Why would it helpful be to me to know my wallet is not in SPV mode if I do have nonvalidated transactions?

If changing the wallet SPV mode toggled the -autorequestblocks behavior, having the mode would begin to make a certain amount of sense, though it also seems it like it would be overselling the SPV feature when the wallet will still be downloading and storing the complete contents of all blocks after a point.

Apart from externally visible names, naming in the code should definitely be made more consistent. If there was a global search and replace in this PR to change all occurrences of "nonvalidation," "nvs," "headers only," "spv," "hybrid," and "auxilliary" with just "nonvalidated" it would help a lot with readability, because the seemingly random choices of names make the implementation seem more haphazard than it needs to be.

ryanofsky commented at 9:58 PM on February 10, 2017: member

To summarize my feedback above, here's what I think ideally would be next steps for this PR:

Get rid of the CAuxiliaryBlockRequest class and integrate prioritized block download logic directly into net_processing.cpp so more code responsible for regular and prioritized block downloads can be shared, and the wallet will not have to be involved in batching and sequencing p2p requests.
Search and replace for occurrences of "nonvalidation," "nvs," "headers only," "spv," "hybrid," and "auxiliary" in new code and just say "nonvalidated" in all the places it makes sense.
Change this PR title from "Complete hybrid full block SPV mode" to something more descriptive and literal like "Support nonvalidated transactions in wallet to increase usability during IBD".
Rename the -spv flag to -priorityrequestblocks or similar to be consistent with -autorequestblocks
Instead of labeling nonvalidated transactions in the wallet as "SPV", label them as "nonvalidated."
Either remove the wallet SPV mode display and toggle until more SPV features are implemented, or extend the SPV mode to toggle both -priorityrequestblocks and -autorequestblocks settings. Having the SPV mode toggle only the -priorityrequestblocks setting while leaving -autorequestblocks set to 1 would be adding a limited feature that's confusing and useless as soon as the initial sync finishes.
Consider defaulting the -priorityrequestblocks flag to true instead of false. There should be few drawbacks if the wallet transactions are clearly labeled as nonvalidated and the wallet can be usable before the IBD completes.
Merge this PR with all the great features it adds. Then follow up with more options for reducing disk and network usage and bring the wallet SPV mode toggle to provide an easy way of enabling them.

jonasschnelli commented at 10:07 AM on July 11, 2017: contributor

@ryanofsky: Thanks for your review and sorry for the late response.

About the idea of getting rid of CAuxiliaryBlockRequest: I think keeping it in a separate file/class allows simpler rebases. I expect to rebase that PR a lot. Also, clustering to much into net_processing would result against in a moster-class/Impl.-file that does everything net related. I think in terms of architecture, splitting of stuff into separate classes/files makes sense.
SPV versus non-validating mode: I haven't really found the ideal term. A first sight, SPV seems to miss the point (if we assume SPV = bloom filter, though I disagree here), but is does allow everybody quickly understand what this PR does. If we look at Satoshi's white paper "Simplified Payment Verification" (chapter 8) then I guess this is more or less what this PR is about. That's why I haven't given up on calling it SPV. Client-mode seems wrong-ish to me, because no "server" is involved. Non-validating mode seems to nail it, but it implies we validate nothing (we still validate headers/PoW) and therefore gives it a negative general direction. Ideally the term should not include what we not do (non) and should be formed in a positive way.

Any objections calling this SPV mode? Also, very likely, this mode will once have client side filtering.

ryanofsky commented at 5:26 PM on July 11, 2017: member

About the idea of getting rid of CAuxiliaryBlockRequest

See #10794 (comment)

Any objections calling this SPV mode?

I don't like it, but I wouldn't object to a useful feature because it has a confusing name, and my complaints above are more about naming inconsistency than about this name in particular. Also, I wish you would respond to my some of my suggestions in detail. I wasn't suggesting renaming "SPV mode" to "non-validating mode" or to "client-mode" (I don't even know where "client-mode" comes from). I suggested renaming the -spv flag to -priorityrequestblocks, to be consistent with -autorequestblocks flag, and because the point of the feature is to be smarter about the order blocks are downloaded.

sipa commented at 5:31 PM on July 11, 2017: member

@ryanofsky In early 2011, there was an incomplete feature in the codebase called "client mode", which probably was intended to be some sort of SPV version. It never got finished, and was eventually removed.

ryanofsky commented at 5:36 PM on July 11, 2017: member

That's interesting. I don't think client is a bad name either (seems pretty innocuous). I just hadn't heard it before.

jonasschnelli moved this from the "In progress" to the "Conceptual PR" column in a project

nopara73 commented at 5:54 PM on November 11, 2017: none

What would be needed to progress this issue further?

jonasschnelli commented at 4:20 AM on November 13, 2017: contributor

I have plans to soon re-do / overhaul the SPV work...

TheBlueMatt commented at 10:07 PM on December 6, 2017: member

Instead of headersChainActive, which seems hard to get to automatically reorg to the new-best-headers-chain after a block is found to be invalid when we get the full block, you may want to take a look at https://github.com/TheBlueMatt/bitcoin/commits/2017-10-best-header-tracking which should handle most of that work for you.

Sjors commented at 7:06 PM on December 11, 2017: member

I'll take a look after rebase. From the description:

requests and persist all blocks that are relevant for the wallet

How does it achieve this? BIP 37? In light of #11863, would it make sense to allow dropping in some arbitrary class that figures out which blocks to request?

laanwj commented at 5:52 PM on December 21, 2017: member

How does it achieve this? BIP 37?

No BIP37 involved here, to retain privacy uses full-block SPV mode. The relevant blocks are only those from the birthdate of the wallet on.

sipa commented at 6:33 PM on March 6, 2018: member

Sorry for the very late comment here, but I think this is introducing a lot of complexity and then building on top of it.

I think a first step should be what @ryanofsky suggested above ("Get rid of the CAuxiliaryBlockRequest class and integrate prioritized block download logic directly into net_processing.cpp so more code responsible for regular and prioritized block downloads can be shared, and the wallet will not have to be involved in batching and sequencing p2p requests."). This is more generally useful than just lightweight mode too; it could be used for rescanning while pruning too, for example.

ryanofsky commented at 6:43 PM on March 6, 2018: member

I think a first step should be what @ryanofsky suggested above ("Get rid of the CAuxiliaryBlockRequest class and integrate prioritized block download logic directly into net_processing.cpp so more code responsible for regular and prioritized block downloads can be shared, and the wallet will not have to be involved in batching and sequencing p2p requests.").

This is actually implemented in #10794. @jonasschnelli, it would probably be good to reference #10794 in the PR description, and make sure the PR description is up to date generally.

ryanofsky commented at 6:45 PM on March 23, 2018: member

@TheBlueMatt pointed out at core dev that the sync implemented here sometimes can't recover from invalid blocks, and that basing this change on #12138 might fix this.

AlbertChanX commented at 12:46 AM on June 5, 2018: none

hey, how to run this version locally? thx

MarcoFalke added the label Needs rebase on Jun 6, 2018

Sjors commented at 10:23 AM on June 15, 2018: member

I'm also in favor of not using the word SPV in the PR description, which does not imply giving up on the term.

Can you clarify this:

Pure full block SPV mode is possible by setting -autorequestblocks=0, in that mode, no blocks for validating the chain will be downloaded, resulting in a SPV only mode.

Did you mean no blocks older than the wallet? If so, how do you check if the UTXO set is correct? If you don't, then maybe the flag should have a scarier name like -partial_utxo_set. Maybe it's better to add that option in a followup PR to improve the worst case security we have to reason about here. @ryanofsky wrote:

But beyond these two things, I don't understand what the wallet "SPV mode" is supposed to indicate. Why would it be helpful to me to know that my wallet is in SPV mode if I don't actually have any nonvalidated transactions? Why would it helpful be to me to know my wallet is not in SPV mode if I do have nonvalidated transactions?

My suggestion would be to show the usual IBD progress bar*, as well as mark transactions as not-fully-validated. In that case there's no need for an extra icon.

= you could go really fancy and draw a thin blue slice at the right most edge to indicate that we have a fragmented chain, using a lighter shade to indicate that it might be completely invalid.

in src/auxiliaryblockrequest.cpp:35 in 254c94ca45

  30 | +    uiInterface.NotifyAuxiliaryBlockRequestProgress(this->created, this->vBlocksToDownload.size(), this->amountOfBlocksLoaded(), this->processedUpToSize);
  31 | +}
  32 | +
  33 | +void CAuxiliaryBlockRequest::processWithPossibleBlock(const std::shared_ptr<const CBlock> pblock, CBlockIndex *pindex)
  34 | +{
  35 | +    // don't process anything if the request was cancled

practicalswift commented at 6:31 PM on September 2, 2018:

Typo found by codespell: cancled

practicalswift commented at 6:34 PM on September 2, 2018:

This should be fixed throughout in this PR :-)

Sjors commented at 8:59 AM on September 3, 2018:

Maybe we need a linter?

practicalswift commented at 9:06 AM on September 3, 2018:

@Sjors Please review #13954 which adds a codespell linter :-)

in src/auxiliaryblockrequest.cpp:69 in 254c94ca45

  64 | +            }
  65 | +        }
  66 | +        this->processedUpToSize++;
  67 | +
  68 | +        // log some info
  69 | +        LogPrint("net", "BlockRequest: proccessed up to %ld of total requested %ld blocks\n", this->processedUpToSize, this->vBlocksToDownload.size());

practicalswift commented at 6:31 PM on September 2, 2018:

Typo found by codespell: proccessed

in src/net_processing.cpp:496 in 254c94ca45

 492 | @@ -481,6 +493,27 @@ void FindNextBlocksToDownload(NodeId nodeid, unsigned int count, std::vector<con
 493 |      // Make sure pindexBestKnownBlock is up to date, we'll need it.
 494 |      ProcessBlockAvailability(nodeid);
 495 |  
 496 | +    // if there is an open CAuxiliaryBlockRequest (out-of-band/specific block donwload), privileg it

practicalswift commented at 6:32 PM on September 2, 2018:

Typo found by codespell: donwload annd privileg

in src/net_processing.cpp:509 in 254c94ca45

 504 | +            if (state->pindexBestKnownBlock == NULL || state->pindexBestKnownBlock->nHeight < pIndexCheck->nHeight)
 505 | +                return false;
 506 | +            return (mapBlocksInFlight.count(pIndexCheck->GetBlockHash()) == 0);
 507 | +        });
 508 | +
 509 | +        // if we haven't completed the individual CAuxiliaryBlockRequest, we wont continue with "normal" IBD

practicalswift commented at 6:33 PM on September 2, 2018:

Typo found by codespell: wont

in src/net_processing.cpp:513 in 254c94ca45

 508 | +
 509 | +        // if we haven't completed the individual CAuxiliaryBlockRequest, we wont continue with "normal" IBD
 510 | +        return;
 511 | +    }
 512 | +
 513 | +    // don't request any other blocks if we are in non autorequest mode (usefull for non-validation mode)

practicalswift commented at 6:35 PM on September 2, 2018:

Typo found by codespell: usefull (also below)

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

There hasn't been much activity lately and the patch still needs rebase, so I am closing this for now. Please let me know when you want to continue working on this, so the pull request can be re-opened.

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

DrahtBot closed this on Dec 3, 2018

BlockMechanic commented at 7:56 PM on September 9, 2019: contributor

Hi

I've always liked the idea of the reference client ie core being available across all platforms. With my work on #16568 proving succeful for android , I'd like to request that an SPV mode be considered seriously for the next major release (0.20). I think ensuring that the core client is accesible across as many platforms (particularly mobile) is a project worth considering.

BlockMechanic commented at 6:02 PM on October 5, 2019: contributor

As we already sync headers , is there a reason to have a separate headers chain ?

laanwj removed the label Needs rebase on Oct 24, 2019

MarcoFalke locked this on Dec 16, 2021