Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add new filter type v0 for segwit only Scripts to blockfilterindex #18223

Closed

Conversation

dangershony
Copy link
Contributor

@dangershony dangershony commented Feb 28, 2020

Introduce a new BlockFilter type [v0] next to the existing basic filter type, which is described in BIP 158.
https://github.com/bitcoin/bips/blob/master/bip-0158.mediawiki#block-filter

Resolves #18222

The BlockFilterIndex implementation allows for the addition of other filter types, this PR will add a new filter type for segwit script programs v0 types only.
Light clients (especially new clients that deal with segwit addresses only) do not need any legacy script types in the filters, and with this PR can choose to only index a much smaller portion of the chain.
That will greatly reduce the amount of data on disk and that is fetch over the network.

The same GCS parameters where used as basic type.

Total size of serialized filers as of block 619 361:

  • Basic = 4.76 GB
  • v0 = 252 MB

Building the Index time

  • Basic = 3.5 hours
  • v0 = 3.5 hours

Enabled in the config by:

blockfilterindex=p2wpkh

Enabling more then one filter:

blockfilterindex=v0
blockfilterindex=basic

Copy link

@nopara73 nopara73 left a comment

Can you add some tests?

src/blockfilter.h Outdated Show resolved Hide resolved
src/blockfilter.cpp Outdated Show resolved Hide resolved
Copy link

@molnard molnard left a comment

PR name: filer => filter
Add new filer type p2wpkh to blockfilterindex
Add new filter type p2wpkh to blockfilterindex

@dangershony dangershony changed the title Add new filer type p2wpkh to blockfilterindex Add new filter type p2wpkh to blockfilterindex Feb 28, 2020
@nopara73
Copy link

@nopara73 nopara73 commented Feb 28, 2020

Note this PR and its accompanying issue (#18221) conforms to the original BIP157 filter extensibility proposal (https://github.com/bitcoin/bips/blob/master/bip-0157.mediawiki#filter-types) and to the proposal I submitted about filter combinations too: #18221

dangershony and others added 2 commits Feb 28, 2020
Co-Authored-By: Dávid Molnár <molnardavid84@gmail.com>
@nopara73
Copy link

@nopara73 nopara73 commented Feb 28, 2020

For the record there is a discussion about that probably it should combine both P2WPKH and P2WSH scripts: #18222 (comment)

@dangershony dangershony changed the title Add new filter type p2wpkh to blockfilterindex Add new filter type v0 for segwit only Scripts to blockfilterindex Mar 1, 2020
Copy link

@nopara73 nopara73 left a comment

tACK. Code LGTM. SegWit v0 filters' size: 151MB.

Copy link
Member

@Sjors Sjors left a comment

Travis is failing do to a whitespace linter issue.

I left some code review comments; it's generally not advisable to spend too much time improving code before a concept ACK. I'm happy to review, as I'm inclined to concept ACK this, but we don't want to be stuck with an abandoned filter type (even though it looks pretty low maintance).

src/blockfilter.cpp Show resolved Hide resolved
src/blockfilter.cpp Show resolved Hide resolved
src/blockfilter.cpp Outdated Show resolved Hide resolved
src/blockfilter.cpp Outdated Show resolved Hide resolved
@Sjors
Copy link
Member

@Sjors Sjors commented Mar 2, 2020

You may want to squash all this into 1 commit (perhaps a separate commit to add OP_RETURN to the basic undo filter).

@dangershony
Copy link
Contributor Author

@dangershony dangershony commented Mar 2, 2020

Thank you @Sjors for your helpful review, I will squash the commits when there is a general ack.

@MarcoFalke
Copy link
Member

@MarcoFalke MarcoFalke commented Mar 3, 2020

Tend to NACK. I fail to see a use case for this, and a missing use case does not justify the time spent on code review and the resulting complexity/user confusion and maintenance overhead when this is merged.

@nopara73
Copy link

@nopara73 nopara73 commented Mar 3, 2020

What do you mean no use case? It's already being used and in production for 1.5 years now.

@luke-jr
Copy link
Member

@luke-jr luke-jr commented Mar 4, 2020

Concept NACK if I understand this correctly.

If we assume everything will be Segwit eventually, this filter will just become redundant with the basic one?

If the goal is to reduce the initial download, couldn't the wallet just start with its birthdate block anyway?

Plus, remotely-accessible filters are still a bad idea in the first place, which this seems to presuppose...

@MarcoFalke
Copy link
Member

@MarcoFalke MarcoFalke commented Mar 4, 2020

What do you mean no use case? It's already being used and in production for 1.5 years now.

If there is a use case internal to the Wasabi wallet, that is fine. However, for anyone else (and probably also Wasabi wallet), they are better off using the existing BIP 158 filters.

src/blockfilter.cpp Outdated Show resolved Hide resolved
src/blockfilter.cpp Outdated Show resolved Hide resolved
src/blockfilter.cpp Show resolved Hide resolved
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this issue Mar 5, 2020
Github-Pull: bitcoin#18223
Rebased-From: dac4d5a
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this issue Mar 5, 2020
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this issue Mar 5, 2020
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this issue Mar 5, 2020
Github-Pull: bitcoin#18223
Rebased-From: 5561e7a
@MarcoFalke
Copy link
Member

@MarcoFalke MarcoFalke commented Mar 5, 2020

even after segwit activation, basic filters are 4.8 GB; compared to 250MB for segwit v0 filters

Segwit usage is higher than 50% right now, and assuming that most wallets have a rather recent birthday, the difference should be a factor of 2 or so (clearly less than 10). Further, assuming that taproot is activated some day, it will likely show a similar or higher usage pattern. With that in mind, promising users storage requirements that can not hold over the long term anyway and converge to those of the all-including basic BIP158 filter, seems slightly misleading.

Finally, network bandwidth shouldn't matter when users feed filters from their local bitcoind instance.

@sipa
Copy link
Member

@sipa sipa commented Mar 5, 2020

If it's for local RPC usage, why is a filter needed at all? You can just fetch the block, and check it for matching transactions like a pre-BIP37 wallet would. If that results in any transactions that were missing from the remotely-provided filter, you know the filter is missing things.

@luke-jr
Copy link
Member

@luke-jr luke-jr commented Mar 5, 2020

Yes @luke-jr, the current way BIP158 is used in Wasabi relies on the server to build and send valid filters - there is no verification, and trust in the server. [this is the problem that we intended to solve with this PR].

This PR cannot solve it.

Wasabi uses block filters solely for the privacy benefits, of having a light wallet without leaking the xpub or address to any third party server.

There are no privacy benefits when you're using your own node.

However, we intended to use SegWit v0 only [later SegWit v1 only] block filters generated in the Wasabi in-built bitcoind, thus every Wasabi user, who opts in to run bitcoind, can cross reference the server filters, from his own generated ones, thus having full verification.

There is no need for a third-party server filter. Just use your own.

@nopara73
Copy link

@nopara73 nopara73 commented Mar 5, 2020

Let me clarify a few things.

Wasabi client uses P2WPKH only filters those are built on and fetched from the Wasabi backend. We want to make Wasabi a hybrid full node, which we've progressed towards gradually, to the point that only the validation function is needed to be implemented. The idea is to serve the exact same filters from the server to the clients, so the client can just verify the correct filters are provided by the server until its own local Bitcoin Core node catches up. Wasabi wallets aren't running all the time, they are closed and reopened constantly, but ultimately what really matters is that eventually the validation happens, the when is less important, IMO.

Porting our own filter building implementation to the client or going through every block for every wallet is no-go as it has poor performance, because going through every block with RPC is a slow process. You may take it as a given as you are working on Bitcoin Core, but the performance of Bitcoin Core is impossible to even come close to for any project.

What just came to my mind, the most important limitation for most performance issues while communicating with Core is that we cannot just pull out the input value-script pars when getting a transaction from Core. Would that be something sensible to start working on? Like a getblock rpc request where more information on the inputs are added to the tx?

@luke-jr
Copy link
Member

@luke-jr luke-jr commented Mar 5, 2020

Like a getblock rpc request where more information on the inputs are added to the tx?

Like #16083 ? (It's already in Knots, so you could even use it immediately!)

@dangershony
Copy link
Contributor Author

@dangershony dangershony commented Mar 5, 2020

@luke-jr that's great, I also noticed you merged the v0 filter commits to Knots, eta when this will go in Knots master?

@luke-jr
Copy link
Member

@luke-jr luke-jr commented Mar 5, 2020

Knots doesn't have a master, just releases. Should be in v0.19.1 (but disabled unless specified explicitly via -blockfilterindex=v0).

@Sjors
Copy link
Member

@Sjors Sjors commented Mar 6, 2020

It may be worth studying c-lightning; they scan all blocks for lightning transactions, no filters needed, and in my experience it's fast. You could wrap some of their C code if it looks too magical :-)

@nopara73
Copy link

@nopara73 nopara73 commented Mar 6, 2020

Like #16083 ? (It's already in Knots, so you could even use it immediately!)

image

@harding
Copy link
Member

@harding harding commented Mar 8, 2020

@MaxHillebrand

Basic filters are unfortunately not feasible for Wasabi [even after segwit activation, basic filters are 4.8 GB; compared to 250MB for segwit v0 filters].

As @MarcoFalke noted above, this conclusion seems unlikely, so I tested the size of the current "basic" filters myself from segwit activation to present and I get a size of 2.6 GB.

$ for i in `seq 481824 620831` ; do bitcoin-cli getblockhash $i ; done | while read hash ; do bitcoin-cli getblockfilter $hash ; done | jq -r .filter | xxd -r -p- | wc -c
2628728815

Beyond that, I'm confused by this disk space argument in the first place. If you're getting the filter for block x from your local node, don't you also expect to be getting the contents of block x from the same node? Blocks are ~33x larger than a full basic filter for that block, so the extra few kilobytes disk space per filter (for present usage patterns) seems particularly irrelevant.

@nopara73

The idea is to serve the exact same filters from the server to the clients, so the client can just verify the correct filters are provided by the server until its own local Bitcoin Core node catches up.

I understand the idea is for the Wasabi application to get identical filters from either Wasabi's server or the local Bitcoin Core node, but I want to make sure you're aware that, as long as the filter encoding is the same, the Wasabi application should be able to use the basic filters produced by Core without any changes to Wasabi's application logic. That's because the application doesn't care whether the filter was constructed out of just P2WPKH transactions or all transactions---it's just looking for potential output/input scriptPubKey matches to its wallet.

So, unless I'm misunderstanding something, the Wasabi application should be able to transition seamlessly between using the P2WPKH-only filters served by the Wasabi server to basic (everything) filters served by the local Bitcoin Core node after it has caught up.

@dangershony
Copy link
Contributor Author

@dangershony dangershony commented Mar 8, 2020

@harding yeah this was incorrect the 4.8GB refers to the entire basic filter data, still core has no way to store locally only basic filters form segwit activation. (also note that 2.6 GB is probably too much if we can avoid it)

the Wasabi application should be able to use the basic filters produced by Core without any changes to Wasabi's application logic.

Wasabi compresses the script on the clients before trying to match it with a filter (or more correctly wasabi builds the filter with compressed scripts) so legacy clients won't work with basic filters from core.

@harding
Copy link
Member

@harding harding commented Mar 8, 2020

2.6 GB is probably too much if we can avoid it

2.6 is roughly 50% the minimum amount of data Bitcoin Core needs to store anyway (chainstate plus minimum prune depth) or the rough equivalent of 10 days worth of blocks and undo data at current utilization levels, so 2.6 GB seems to me to be a pretty small additional requirement on top of using Bitcoin Core in the first place.

And, as mentioned elsewhere in this thread, you can use the wallet birthday to prune filters from blocks before the wallet was created (though I don't think we currently support filter pruning).

wasabi builds the filter with compressed scripts) so legacy clients won't work with basic filters from core.

I'm confused. What are "compressed scripts"? I skimmed this PR---it is a nice example of a minimalistic change, thanks!---and it seemed to me that it builds both the basic and v0 filters the same way, except that the v0 filter skips past any transactions with non-v0 parts:

if (!script.IsWitnessProgram(witnessversion, witnessprogram)) 
if (witnessversion != witness_version) 
if (!(witnessversion == 0 && (witnessprogram.size() == WITNESS_V0_KEYHASH_SIZE || witnessprogram.size() == WITNESS_V0_SCRIPTHASH_SIZE))) continue; // specific v0 checks

Unless I'm missing something, that should mean that any Wasabi clients compatible with this PR would also automatically be compatible with basic filters with no changes needed (except maybe to deal with the break in the chain of filter headers).

@dangershony
Copy link
Contributor Author

@dangershony dangershony commented Mar 9, 2020

You are right, this filter will sill not be compatible with wasabi (and had we gone down this path would mean we have to support two api endpoints after the upgrade) there are a few narratives in parallel here (fetch filters from local core node, fetch filters from wasabi server, maintain backwards compatibility, least disruption to current wasabi design etc...) this will explain the confusion.

so 2.6 GB seems to me to be a pretty small additional requirement on top of using Bitcoin Core in the first place.

Each client (not using a local core node) will fetch 2.6GB of data form the server (wasabi stores all filters locally) if we can prevent this its preferable.

Right new it seems we probably take the approach of using Knots and constructing the same filters wasabi currently uses from a Knots node (that can serve the spent ScriptPubKey)
zkSNACKs/WalletWasabi#3208 (comment)

FYI compressed scripts to bytes https://github.com/zkSNACKs/WalletWasabi/blob/master/WalletWasabi/Blockchain/BlockFilters/IndexBuilderService.cs#L264

@harding
Copy link
Member

@harding harding commented Mar 9, 2020

Each client (not using a local core node) will fetch 2.6GB of data form the server (wasabi stores all filters locally) if we can prevent this its preferable.

Yes, this can be prevented, that's what I've been trying to say but I must not be communicating effectively here, so let me start from first principles (my appologies for repeating stuff you probably already know).

The process of creating a BIP158-style filter starts with a block that's full of transactions which each spend one or more UTXOs and which each create one or more new UTXOs. For BIP158, scriptPubKeys for all spent UTXOs and created UTXOs (excepting OP_RETURN outputs) are hashed and then put into a sorted list:

00000000
11111111
22222222
33333333

The list is then golumb coded to minimize its size to produce the final BIP158 basic filter. The client then takes a list of its scriptPubKeys, hashes them the same way, and compares each of them to the entries in the filter to see if there's any matches. E.g., the wallet matches on 22222222 and so goes and downloads the block to find the possibly-relevant transaction.

Now, as proposed in this PR, you can create a filter using a subset of the original data (e.g. only P2WPKH):

11111111
22222222

If you use the same hashing and golumb coding, then any client which can process the subset filter above can also process the original full set filter to find its scriptPubKey 22222222.

This means Wasabi's server can generate and serve the small subset filter (~250 MB currently you say) while it's also possible for clients to use their local node's full filter (~2.6 GB post-segwit activation). This is my point: you don't have to choose between a 250 MB and a 2.6 GB filter---you can use them both in different contexts with no special logic in the client.

Right new it seems we probably take the approach of using Knots and constructing the same filters wasabi currently uses from a Knots node (that can serve the spent ScriptPubKey)

If you do indeed have a custom GCS algorithm, that does seem like it might be the simplest path forward for you, but it seems a shame to me that choosing that path would remove the possibility of compatibility with the most popular and best reviewed full node implementation (as well as all other software that's implemented standard BIP158).

FYI compressed scripts to bytes https://github.com/zkSNACKs/WalletWasabi/blob/master/WalletWasabi/Blockchain/BlockFilters/IndexBuilderService.cs#L264

That looks like it's just doing the GCS compression; I'm not sure how your code differs from the same operation as described in BIP158.

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this issue Oct 10, 2021
luke-jr added a commit to bitcoinknots/bitcoin that referenced this issue Oct 10, 2021
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this issue Oct 10, 2021
Co-Authored-By: Dávid Molnár <molnardavid84@gmail.com>

Github-Pull: bitcoin#18223
Rebased-From: 7044815
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this issue Oct 10, 2021
luke-jr added a commit to bitcoinknots/bitcoin that referenced this issue Oct 10, 2021
luke-jr added a commit to bitcoinknots/bitcoin that referenced this issue Oct 10, 2021
Github-Pull: bitcoin#18223
Rebased-From: dac4d5a
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this issue Oct 10, 2021
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this issue Oct 10, 2021
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this issue Oct 10, 2021
Github-Pull: bitcoin#18223
Rebased-From: 5561e7a
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.