Complete hybrid full block SPV mode

[jonasschnelli]

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:

[luke-jr]

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

[gmaxwell]

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]

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

[dabura667]

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]

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

[dabura667]

@luke-jr I would think so, yes.

[molxyz]

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

[diegoviola]

@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]

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

[luke-jr]

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

[jonasschnelli]

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]

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]

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.

[jonasschnelli]

Adapted to work with bumpfee.
Added a fix for the UI update issue reported by @molxyz and @diegoviola (#9483 (comment)).

[ryanofsky]

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

[ryanofsky]

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.

[ryanofsky]

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

[ryanofsky]

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

[ryanofsky]

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

[ryanofsky]

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.

[ryanofsky]

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

[ryanofsky]

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.

[ryanofsky]

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

[ryanofsky]

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.

[ryanofsky]

Doxygen comment is now out of date.

[ryanofsky]

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

[ryanofsky]

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.

[ryanofsky]

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]

Good point.

[ryanofsky]

Maybe rename fFetchBlocksWhileFetchingHeaders to fRequestBlocksWhileFetchingHeaders for consistency with fAutoRequestBlocks.

[ryanofsky]

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";
} ...

[ryanofsky]

Probably should throw if no arguments passed.

[ryanofsky]

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]

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

[ryanofsky]

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

[ryanofsky]

Maybe avoid copy with const &

[ryanofsky]

Could use make_shared.

[ryanofsky]

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

[jonasschnelli]

Would returning true be guaranteed then?

[ryanofsky]

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.

[ryanofsky]

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

[ryanofsky]

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.

[ryanofsky]

Method looks like it could be const.

[ryanofsky]

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.

[ryanofsky]

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

[ryanofsky]

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

[ryanofsky]

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

[ryanofsky]

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

[ryanofsky]

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

[ryanofsky]

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

[ryanofsky]

Maybe for (const auto& entry : mapKeyBirth)

[ryanofsky]

Maybe it->second

[ryanofsky]

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.

[ryanofsky]

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.

[ryanofsky]

Maybe use make_shared.

[ryanofsky]

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

[ryanofsky]

Maybe should throw if no arguments passed.

[ryanofsky]

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

[ryanofsky]

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);
}

[ryanofsky]

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

[ryanofsky]

spelling "useful"

[ryanofsky]

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]

Should 600 be 3600?

[ryanofsky]

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.

[ryanofsky]

spelling: several

[ryanofsky]

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:

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

2. 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]

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]

@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]

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]

@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]

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