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

Added support for MSG_FILTERED_WITNESS_BLOCK messages. #10350

Closed

Conversation

@CodeShark
Copy link
Contributor

@CodeShark CodeShark commented May 7, 2017

After much deliberation and a few attempts at other approaches to provide a workable near-term sync mechanism for thin clients that require witness data, I decided to just go with the simplest short-term path with the least amount of complications expecting BIP37 to be completely replaced eventually - hopefully in the not-very-distant future.

I believe the approach of filtering blocks on the client is a simpler and superior approach for most use cases than requiring the server to perform the filtering. I believe @Roasbeef has something written up for this that he's using for lnd. I would love to see that approach used in Bitcoin Core as well.

But given the good likelihood of nearterm SegWit activation on the Bitcoin mainnet, I believe this solution will suffice for all essential use cases of BIP37 for now - and I don't believe it's worth the effort to try to make more complex additions to BIP37 since it will eventually be entirely replaced.

Peers can request MSG_FILTERED_WITNESS_BLOCK and will receive a merkleblock structure with transactions serialized with witnesses. The merkle proof for the witnesses is not supplied. This means that the client cannot verify that the witness data is what's actually in the block. However, the attack vectors here given the actual intended use cases seem extremely slim for several reasons:

  1. The witness data contains signatures which the client can still verify. Spoofing the witness would require supplying signatures that still redeem the output, meaning that only parties that can sign for the output could produce false witness data.

  2. In order to use BIP37 with any real degree of privacy and security, you need to connect to a trusted node. If this is your setup, adding merkle proofs for witnesses is an unnecessary complication.

  3. The foreseen intended use cases here are wallets that support multisignature scripts or scripts with multiple execution paths where you want to be able to check which signatures are provided or which execution path has been taken. In anticipated use cases, there is not much an attacker could gain from a transaction being signed in two different ways - and typically, the attacker would be easily identifiable.

@gmaxwell
Copy link
Contributor

@gmaxwell gmaxwell commented May 7, 2017

Why trash privacy, create additional CPU attack vectors, dos attack vectors for the client, etc. to save at most 26kbit/sec? It doesn't seem like a useful tradeoff.

@CodeShark
Copy link
Contributor Author

@CodeShark CodeShark commented May 7, 2017

Bandwidth isn't the main concern. Code complexity is. It's only intended to be a very short-term solution until we can introduce clientside block filtering.

@CodeShark
Copy link
Contributor Author

@CodeShark CodeShark commented May 7, 2017

I should add that using BIP37 with untrusted nodes is already quite insecure.

@TheBlueMatt
Copy link
Contributor

@TheBlueMatt TheBlueMatt commented May 7, 2017

I also dont see the rush...SegWit isn't gonna activate any time soon, why extend the current crap isntead of replacing it properly when there is no rush?

@CodeShark
Copy link
Contributor Author

@CodeShark CodeShark commented May 7, 2017

@TheBlueMatt It's already active on testnet as well as on several alts which seek to minimize the code diff.

@TheBlueMatt
Copy link
Contributor

@TheBlueMatt TheBlueMatt commented May 7, 2017

@CodeShark testnet is for testing Bitcoin, I dont generally consider it a target for features? As you point out this is a rather simple patch, if some alts have a need for it, they can easily take and maintain this patch.

@CodeShark
Copy link
Contributor Author

@CodeShark CodeShark commented May 8, 2017

My entire stack currently relies on BIP37 for synching with a node, including a number of testing tools I use. The alternatives of either maintaining a custom server (one more thick dependency) or using RPC (excruciatingly and impractically slow and inefficient) aren't really options for me at this point. So until we do something like clientside block filtering I'm left with either using this patch or having to hack something up that's really ugly on my end.

I understand that there's a desire to not pollute the codebase with stuff that most people probably will not use - so I fully understand if other devs are reluctant to want to merge this. But I do not believe it is particularly intrusive - it doesn't really get in the way of anyone else and is easy to review and test for correctness. And it would save at least some people a nontrivial amount of additional effort in having to maintain separate branches and patches.

I'm eager to contribute to the effort of finding a good long-term solution to the thin client sync issue and helping to implement it. But right now I need to make some important software releases, and the more I need to worry about my own software's short-term compatibility issues the less I can focus on the bigger picture.

Anyhow, I appreciate the consideration.

@fanquake fanquake added the P2P label May 8, 2017
@CodeShark CodeShark force-pushed the CodeShark:MSG_FILTERED_WITNESS_BLOCK_2 branch from 1c8b67d to fcf7d60 Aug 12, 2017
@MarcoFalke
Copy link
Member

@MarcoFalke MarcoFalke commented Aug 25, 2018

Closing for now. Let me know when you want to continue working on this

@MarcoFalke MarcoFalke closed this Aug 25, 2018
@laanwj laanwj removed the Needs rebase label Oct 24, 2019
fanquake added a commit that referenced this pull request Aug 18, 2020
4792cad doc: comment out and add annotation to unused MSG_FILTERED_WITNESS_BLOCK (Adam Jonas)

Pull request description:

  Commenting out and adding a note to unused `MSG_FILTERED_WITNESS_BLOCK` [defined in BIP144](https://github.com/bitcoin/bips/blob/master/bip-0144.mediawiki#relay).

  There was an attempt to make use of this in #10350, but it was closed due to lack of support. (h/t sdaftuar for pointing to the PR and jnewbery for the idea)

ACKs for top commit:
  jnewbery:
    Obvious ACK 4792cad
  theStack:
    ACK 4792cad 📜
  MarcoFalke:
    cr ACK 4792cad good to keep it around in a comment to avoid accidental future re-assignment
  practicalswift:
    ACK 4792cad

Tree-SHA512: 22327ddded643ae50fdb529e4529a9b464f74e90620d0d2079a11070eaa8afe8363f6e14cca52f3bec2c9f87ee13e318edc6c5193761c94b8ae77be353a8da1f
sidhujag added a commit to syscoin/syscoin that referenced this pull request Aug 18, 2020
4792cad doc: comment out and add annotation to unused MSG_FILTERED_WITNESS_BLOCK (Adam Jonas)

Pull request description:

  Commenting out and adding a note to unused `MSG_FILTERED_WITNESS_BLOCK` [defined in BIP144](https://github.com/bitcoin/bips/blob/master/bip-0144.mediawiki#relay).

  There was an attempt to make use of this in bitcoin#10350, but it was closed due to lack of support. (h/t sdaftuar for pointing to the PR and jnewbery for the idea)

ACKs for top commit:
  jnewbery:
    Obvious ACK 4792cad
  theStack:
    ACK bitcoin@4792cad 📜
  MarcoFalke:
    cr ACK 4792cad good to keep it around in a comment to avoid accidental future re-assignment
  practicalswift:
    ACK 4792cad

Tree-SHA512: 22327ddded643ae50fdb529e4529a9b464f74e90620d0d2079a11070eaa8afe8363f6e14cca52f3bec2c9f87ee13e318edc6c5193761c94b8ae77be353a8da1f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.