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

Verify merkle root and block headers to help #297 #334

Merged
merged 13 commits into from Sep 2, 2020

Conversation

mflaxman
Copy link
Contributor

@mflaxman mflaxman commented Aug 31, 2020

One of the few negatives of Specter (vs Electrum) is that users have to sync a full node before getting started.

It would be easier if less security-concerned users could use a cloud-hosted bitcoin node for consensus rules (it might be a trusted friend's node for example), at least to get started.

However, one big concern is when you accept a payment if you're interacting with the real bitcoin blockchain. It would be easy for a malicious full node to lie to you and claim you received a payment that you never did (and your hardware wallets would be unable to verify this). The easy way to get around that now is to search block explorers for your address, but this has obvious privacy issues (half the reason Specter requires a full-node).

This simple PR validates the merkle root and block headers, so we're able to prove with certainty that a transaction was included in a a specific block hash. If we can search block explorers (or other full nodes) to see that this block hash made it into the bitcoin blockchain, then we know that transaction did as well. It's kind of like a human version of Neutrino in that sense.

I wasn't sure what to do about the UI, but I did pass in an explicit tx['validate_merkle_root'] = True that could be used for an advanced/hidden UI where you display that transaction X was in block Y (tx['blockhash']).

It doesn't feel slower to me, but in case performance is a concern it might make sense to have a setting to disable this feature if desired?

def merkle_parent_level(hash_list):
'''Takes a list of binary hashes and returns a list that's half
the length'''
# Exercise 2.2: if the list has exactly 1 element raise an error
if len(hash_list) == 1:
raise RuntimeError('Cannot take a parent level with only 1 item')
# Exercise 3.2: if the list has an odd number of elements, duplicate the
# last one and put it at the end so it has an even number
# of elements
if len(hash_list) % 2 == 1:
hash_list.append(hash_list[-1])
# Exercise 2.2: initialize next level
parent_level = []
# Exercise 2.2: loop over every pair
# (use: for i in range(0, len(hash_list), 2))
for i in range(0, len(hash_list), 2):
# Exercise 2.2: get the merkle parent of i and i+1 hashes
parent = merkle_parent(hash_list[i], hash_list[i+1])
# Exercise 2.2: append parent to parent level
parent_level.append(parent)
# Exercise 2.2: return parent level
return parent_level
Copy link
Contributor

@PulpCattel PulpCattel Aug 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def merkle_parent_level(hash_list):
'''Takes a list of binary hashes and returns a list that's half
the length'''
# Exercise 2.2: if the list has exactly 1 element raise an error
if len(hash_list) == 1:
raise RuntimeError('Cannot take a parent level with only 1 item')
# Exercise 3.2: if the list has an odd number of elements, duplicate the
# last one and put it at the end so it has an even number
# of elements
if len(hash_list) % 2 == 1:
hash_list.append(hash_list[-1])
# Exercise 2.2: initialize next level
parent_level = []
# Exercise 2.2: loop over every pair
# (use: for i in range(0, len(hash_list), 2))
for i in range(0, len(hash_list), 2):
# Exercise 2.2: get the merkle parent of i and i+1 hashes
parent = merkle_parent(hash_list[i], hash_list[i+1])
# Exercise 2.2: append parent to parent level
parent_level.append(parent)
# Exercise 2.2: return parent level
return parent_level
def merkle_parent_level(hash_list):
'''Takes a list of binary hashes and returns a list that's half
the length'''
# If the list has exactly 1 element raise an error
if len(hash_list) == 1:
raise RuntimeError('Cannot take a parent level with only 1 item')
# If the list has an odd number of elements, duplicate the
# last one and put it at the end so it has an even number
# of elements
if len(hash_list) % 2 == 1:
hash_list.append(hash_list[-1])
# Initialize next level
parent_level = []
# Loop over every pair
for i in range(0, len(hash_list), 2):
# Get the merkle parent of i and i+1 hashes
parent = merkle_parent(hash_list[i], hash_list[i+1])
# Append parent to parent level
parent_level.append(parent)
# Return parent level
return parent_level

Imho is nice to remove Exercise from all the comments. I'm not sure why they are in the Jimmy Song's repo in the first place.

Copy link
Collaborator

@stepansnigirev stepansnigirev left a comment

I think this feature should be disabled by default. For autoconfig and local nodes - for sure, for remote nodes can be enabled via a checkbox in bitcoin core settings. Maybe something like "Don't trust this node" or something.

def merkle_parent_level(hash_list):
'''Takes a list of binary hashes and returns a list that's half
the length'''
# Exercise 2.2: if the list has exactly 1 element raise an error
Copy link
Collaborator

@stepansnigirev stepansnigirev Aug 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove "Exercise N.N" comments

# Cache queries to be safe:
blockhash = tx['blockhash']
if blockhash not in blocks:
blocks[blockhash] = self.cli.getblock(blockhash)
Copy link
Collaborator

@stepansnigirev stepansnigirev Aug 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will fail on a pruned node for most of the transactions.
Also why not to use gettxoutproof? Then you don't need the block, only the block header.

@stepansnigirev
Copy link
Collaborator

stepansnigirev commented Aug 31, 2020

At the current state this PR is not useful - you are verifying only that tx is in the block, but you are not checking that this block is in the blockchain.

To get some security benefit from this PR we would need to

  1. Download all block headers from Bitcoin Core node
  2. Validate all block headers
  3. Validate merkle proofs from Bitcoin Core for every transaction

And this basically means building a light client in Specter. But then why would you rely on Bitcoin Core to calculate the balance of the wallets if you don't trust the node?

Integration of the light wallet to Specter should be more modular - I think it should be a separate backend that mimics bitcoin core's rpc, can manage wallets and work with descriptors similar to Bitcoin Core, but works as a light client. Then Specter logic is not touched at all, and the wallet backend can be anything - electrum, neutrino, block explorer - as soon as they implement the same interface.

@mflaxman
Copy link
Contributor Author

mflaxman commented Aug 31, 2020

Thanks for the feedback @stepansnigirev and @PulpCattel!

I made the coding changes: got rid of exercise comments, switched to gettxoutproof (good call), etc... That's somewhat secondary to the main feedback from @stepansnigirev. Sorry, I should've clarified before that I wanted concept review and not merging.

To be useful this PR would need to surface the blockhash that was validated against so that the end user could verify that blockhash exists in the bitcoin blockchain themselves. I'm thinking something like a column here (perhaps with a mouseover explainer):
Screen Shot 2020-08-31 at 5 46 49 PM

The idea is that you'd then search for this blockhash, which does reveal some privacy info (why do you care about this particular block hash?), but is orders of magnitude better than searching a block explorer for an address or transaction hash. It would definitely need some sort of explainer as an advanced-only feature.

If that's useful, I'm happy to add test coverage, make it optional (default off) and come up with some sort of UI/layout.

@stepansnigirev, in addition to your list of 3 things needed for security benefits in a light client, a system like that would have to ping out to other nodes to check the difficulty/work! Without that, it'd be easy for a malicious hosted node to construct fake blocks with extremely low difficulty (that is technically valid).

I'd be a fan of a modular verification service, but I think of that as something separate. Merkle proof verification is stateless+fast, and allows you to go from searching a TX to searching a block hash to (re)confirm that your (online!) node isn't malicious. If you're using Specter-Desktop to go to extreme lengths to keep keys offline, then it seems strange to trust an online machine to validate a received payment.

The magic of the blockchain structure is that having the same blockhash means you agree on all previous transaction history up to that point.

@stepansnigirev
Copy link
Collaborator

stepansnigirev commented Sep 1, 2020

Ok, I understand now. I think having a link to the block is a good idea that improves privacy.
And the Merkle proof verification can be a good first step for a lighter client.

@mflaxman
Copy link
Contributor Author

mflaxman commented Sep 1, 2020

Some screenshots, how's this?

Default (turned off) is same as before (I'd rather not show unverified data when there's no need):
Screen Shot 2020-09-01 at 5 12 50 PM

Settings page (to turn on):
Screen Shot 2020-09-01 at 5 11 54 PM

What it looks like (copy feature works):
Screen Shot 2020-09-01 at 5 11 29 PM

Tooltip (links to settings page to disable):
Screen Shot 2020-09-01 at 5 11 33 PM

@mflaxman
Copy link
Contributor Author

mflaxman commented Sep 1, 2020

Test coverage added in 00f82d7!:

p$ pytest tests/test_merkleblock.py -v
====================================================== test session starts ======================================================
platform darwin -- Python 3.8.5, pytest-5.2.2, py-1.9.0, pluggy-0.13.1 -- /Users/mflaxman/workspace/specter-desktop/.venv3/bin/python
cachedir: .pytest_cache
rootdir: /Users/mflaxman/workspace/specter-desktop, inifile: pytest.ini
collected 14 items                                                                                                              

tests/test_merkleblock.py::BlockTest::test_bip141 PASSED                                                                  [  7%]
tests/test_merkleblock.py::BlockTest::test_bip9 PASSED                                                                    [ 14%]
tests/test_merkleblock.py::BlockTest::test_bip91 PASSED                                                                   [ 21%]
tests/test_merkleblock.py::BlockTest::test_check_pow PASSED                                                               [ 28%]
tests/test_merkleblock.py::BlockTest::test_hash PASSED                                                                    [ 35%]
tests/test_merkleblock.py::BlockTest::test_parse PASSED                                                                   [ 42%]
tests/test_merkleblock.py::BlockTest::test_serialize PASSED                                                               [ 50%]
tests/test_merkleblock.py::BlockTest::test_target PASSED                                                                  [ 57%]
tests/test_merkleblock.py::BlockTest::test_validate_merkle_root PASSED                                                    [ 64%]
tests/test_merkleblock.py::MerkleBlockTest::test_is_valid PASSED                                                          [ 71%]
tests/test_merkleblock.py::MerkleBlockTest::test_parse PASSED                                                             [ 78%]
tests/test_merkleblock.py::MerkleTreeTest::test_init PASSED                                                               [ 85%]
tests/test_merkleblock.py::MerkleTreeTest::test_populate_tree_1 PASSED                                                    [ 92%]
tests/test_merkleblock.py::MerkleTreeTest::test_populate_tree_2 PASSED                                                    [100%]

====================================================== 14 passed in 0.03s =======================================================

If the functionality seems right, I think this is ready to merge.

@@ -13,18 +15,39 @@
<th class="optional">Address</th>
<th>Amount</th>
<th>Status</th>
{% if specter.config.validate_merkle_proofs %}
<th>
Copy link
Contributor

@ben-kaufman ben-kaufman Sep 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<th>
<th class="optional">

Just need this not to break mobile view

Copy link
Contributor Author

@mflaxman mflaxman Sep 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure, the amount/status aren't marked class="optional"? This whole section only appears if you have specter.config.validate_merkle_proofs.

Copy link
Contributor

@ben-kaufman ben-kaufman Sep 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I'm sure, the amounts and status are small enough to fit in mobile display, but the block header is too large... you can include it in the mobile-only pop up which has the txid and all, but don't have to if you find it confusing to implement.

Copy link
Contributor Author

@mflaxman mflaxman Sep 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, that makes sense. Thanks for the detailed explanation, I've barely used jinja before!

Copy link
Contributor Author

@mflaxman mflaxman Sep 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Separately, maybe the class should be renamed to something like mobile-compatible, collapsible, squeezable, etc in the future? Optional expresses something a little different (that the value is not required), but there are formatting benefits when it's guaranteed.

Copy link
Contributor

@ben-kaufman ben-kaufman Sep 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that definitely make sense, I'm not great with naming lol

@@ -71,6 +71,13 @@
</select>
<br><br>
{% endif %}
<div class="row">
<label>Validate Merkle Proofs:&nbsp;</label>
Copy link
Contributor

@ben-kaufman ben-kaufman Sep 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe also add tooltip here to explain what this is?

Copy link
Collaborator

@stepansnigirev stepansnigirev left a comment

Some changes required to handle unconfirmed transactions and pruned nodes properly.
Maybe disable this feature if the node is pruned and notify the user that merkle proof verification doesn't work on a pruned node.
And don't try to verify for unconfirmed transactions.

txids.append(tx["txid"])
tx['validated_blockhash'] = "" # default is assume unvalidated
if validate_merkle_proofs is True:
proof_hex = self.cli.gettxoutproof([tx['txid']])
Copy link
Collaborator

@stepansnigirev stepansnigirev Sep 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can cause an exception for unconfirmed transactions and for confirmed transactions in pruned nodes when the block is pruned.

Copy link
Contributor Author

@mflaxman mflaxman Sep 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point re unconfirmed TXs, but are you sure re pruned nodes? I just updated my PR to add the blockhash, which should always work:

https://bitcoincore.org/en/doc/0.20.0/rpc/blockchain/gettxoutproof/

NOTE: By default this function only works sometimes. This is when there is an
unspent output in the utxo for this transaction. To make it always work,
you need to maintain a transaction index, using the -txindex command line option or
specify the block in which the transaction is included manually (by blockhash).

I'm trying to test a pruned node, but I accidentally triggered a rescan so mine is out for a while :(

Copy link
Collaborator

@stepansnigirev stepansnigirev Sep 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually it doesn't even if there are unspent outputs. That's the reason we had to fetch proofs from blockexplorer in #344
Just checked on my pruned node, getting an rpc error, even though this transaction is imported to the wallet with the proof.

error code: -32603
error message:
Can't read block from disk

Copy link
Contributor Author

@mflaxman mflaxman Sep 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, didn't realize the intricacies of pruned nodes. I assumed they kept all blocks that they had transactions in (or at least the merkle proofs). I wonder what the thought process was there?

I'll update the PR with disabling it altogether for pruned nodes.

@ben-kaufman
Copy link
Contributor

ben-kaufman commented Sep 2, 2020

Also seems not to be working on the "wallets overview" screen and the UTXO view. Would be good to have it there properly too (let me know if you need help with that).

@mflaxman
Copy link
Contributor Author

mflaxman commented Sep 2, 2020

Also seems not to be working on the "wallets overview" screen and the UTXO view. Would be good to have it there properly too (let me know if you need help with that).

@ben-kaufman can you try again? Wallets overview should work now, but for UTXO I had to specifically not enable merkle proofs. That page uses a different code path (not txlist() nor full_txlist()) so the process of adding merkle proof support to that page would touch a ton of things and I want it to remain DRY. wallet.utxo is set on getdata(), which is called all over the codebase. Perhaps it would make sense to combine this with txlist in the future somehow? I haven't really dug into how that part of the codebase works, but it seems like it'd be a refactor and I wouldn't want that to block this.

@ben-kaufman
Copy link
Contributor

ben-kaufman commented Sep 2, 2020

Makes sense. The UTXO part probably needs some refactoring anyway, but that's a story for another PR :)
Besides just adding the optional class so it looks good on mobile (feel free to rename this to something which makes more sense if you want), and for me it looks good (I didn't check on a pruned node or with unconfirmed transactions though).

target_block_hash_hex=tx['blockhash'],
target_merkle_root_hex=None,
):
# NOTE: this does NOT guarantee this blockhash is actually in the bitcoin blockchain!
Copy link
Contributor

@PulpCattel PulpCattel Sep 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# NOTE: this does NOT guarantee this blockhash is actually in the bitcoin blockchain!
# NOTE: this does NOT guarantee this blockhash is actually in the Bitcoin's blockchain!

Makes it consistent

Copy link
Contributor Author

@mflaxman mflaxman Sep 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, thanks!

@mflaxman
Copy link
Contributor Author

mflaxman commented Sep 2, 2020

@stepansnigirev the feature is now disabled on pruned nodes.

Any other changes to make? I think it's looking good!

mflaxman and others added 2 commits Sep 2, 2020
Co-authored-by: Luke <51127079+PulpCattel@users.noreply.github.com>
@stepansnigirev
Copy link
Collaborator

stepansnigirev commented Sep 2, 2020

Looks good! Merging :)

@stepansnigirev stepansnigirev merged commit 5b56052 into cryptoadvance:master Sep 2, 2020
@mflaxman
Copy link
Contributor Author

mflaxman commented Sep 2, 2020

Thanks everyone for the code review, feedback, copy changes, help, etc!

Lol, I thought this would be a simple one but turned out the crypto was the easy part.

I'm hoping this starts making hosted nodes more of a thing (less barrier to using Specter-Desktop), although a bummer that these hosted nodes can't be pruned. That seems like a mistake in the current bitcoin core implementation of gettxoutproof (see here).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants