hash (features|commitment) in output mmr #615

antiochp · 2018-01-15T03:54:20Z

This PR solves #559 by extending the output pmmr to include the output features in the hash.

The proposed solution is described here in more detail - #597

Replaces #215 (which attempted to encode lock_height in the switch commit)

tl;dr

Getting rid of the indexes ensures we never see the "wrong" version of an output (due to a fork).
Specifically we never see (and use) the wrong lock_height (which can and will break consensus).
We now include output features in the output mmr. This ensures a miner cannot lie about coinbase vs non-coinbase (we can be sure we are checking lock_height as necessary)
We derive lock_height by looking at the full block (we do not rely on the old index) so we know lock_height is being correctly handed

The combination of these changes should ensure we handle the spending of coinbase outputs correctly even under adverse conditions (chain forks and miners reusing wallet keys, creating duplicate commitments).

SumCommit takes OutputFeatures & Commitment
- the hash is now (features|commit)
- this ensures a given output in the MMR is known to be a coinbase or not (and miner cannot lie)
introduce OutputIdentifier so we don't need to pass a SumCommit around everywhere
- we now need an OutputIdentifier to check is_unspent via the MMR (by comparing the hash)
to spend an output we also need to specify the block hash in the Input
- we can determine lock_height based on height of block + 1,000
- miner cannot lie about lock_height (also cannot lie about coinbase vs non-coinbase)
removed "header by commit" index
- sender needs to provide block hash
- wallet maintains block hash of outputs
- this ensures we never mis-attribute an output in the presence of a chain fork
removed "output by commit" index
- if we need the full output we need to go back to the full block (via block hash)
get utxo api endpoint now just returns a commitment (output mmr data only)
rework wallet refresh and restore to handle output mmr changes and need to maintain block hashes

TODO -

many tests failing (tests failing to build)
many build warnings, lack of docs, unused imports etc.
cleanup verbose debug logging
make block hash optional on an input (only necessary for spending coinbase outputs)

workaround this for wallet refresh and wallet restore

switch commit hash key encodes lock_height cleanup output by commit index (currently broken...)

…commit_hashes

utxos come from the sumtrees (and only the sumtrees, limited info) outputs come from blocks (and we need to look them up via block height)

this uses sumtree in a consistent way

antiochp · 2018-01-16T02:13:30Z

@ignopeverell this should be in a reasonable place for an early review.
Tests are still failing... and there are a lot of build warnings... but the approach outlined in #597 is implemented (and works locally).
If we are happy with this and think this is worth continuing with then I'll continue to clean it up and get the tests reworked/fixed up.

chain/src/sumtree.rs

+						// edge case here - we may be spending an output with zero-confirmations
+						// and it just got included in a block.
+						// The wallet may not know about this new block yet (not yet refreshed).
+						// Is it safe to assume this the tx was included in the latest block?


chain/src/sumtree.rs

+				// If we are spending a coinbase output then go find the block
+				// and check the coinbase maturity rule is being met.
+				if input.features.contains(COINBASE_OUTPUT) {
+					// edge case here - we may be spending an output with zero-confirmations


chain/tests/test_coinbase_maturity.rs

@@ -176,6 +183,23 @@ fn test_coinbase_maturity() {

 	let prev = chain.head_header().unwrap();

+<<<<<<< HEAD


ignopeverell · 2018-01-16T06:43:40Z

Minor comments but looks good to me overall!

…coinbase outputs)

antiochp · 2018-01-16T17:17:27Z

@ignopeverell this should be good to go now if you want to give it a final review?
(3rd time lucky - but this feels like the "right" solution now). 😀

ignopeverell · 2018-01-16T19:03:19Z

Looking good, ship it!

antiochp · 2018-01-16T20:26:39Z

Soon as travis sorts out its infra... multiple builds stuck for a couple of hours. Just kicked off a new one.
Edit: still waiting. Looks like they have a huge backlog of linux builds queued up...

antiochp · 2018-01-17T03:03:25Z

Merging - travis is still working through its backlog...

antiochp added 30 commits January 4, 2018 14:29

experiment with lock_heights on outputs

9a193fd

playing around with lock_height as part of the switch commitment hash

1e21029

cleanup

1bb9ed7

include features in the switch commit hash key

d13b39b

commit

ba22ed5

rebase off master

1267fd5

commit

13acb42

cleanup

8de1189

missing docs

27ca435

rework coinbase maturity test to build valid tx

70a99f4

pool and chain tests passing (inputs have switch commitments)

3bab309

commit

60f8c0b

cleanup

f8e5d35

check inputs spending coinbase outputs have valid lock_heights

1c46b86

wip - got it building (tests still failing)

ab41029

use zero key for non coinbase switch commit hash

6abdc25

fees and height wrong order...

6861862

send output lock_height over to wallet via api

2db64ff

no more header by height index

22a04d5

workaround this for wallet refresh and wallet restore

refresh heights for unspent wallet outputs where missing

47e4719

TODO - might be slow?

4d3b48c

simplify - do not pass around lock_height for non coinbase outputs

ced54ec

commit

1d2cb4e

Merge branch 'master' into lockheight_outputs

ca22c40

fix tests after merge

94f70f5

build input vs coinbase_input

057d581

switch commit hash key encodes lock_height cleanup output by commit index (currently broken...)

is_unspent and get_unspent cleanup - we have no outputs, only switch_…

5c5fe39

…commit_hashes

separate concept of utxo vs output in the api

6598d45

utxos come from the sumtrees (and only the sumtrees, limited info) outputs come from blocks (and we need to look them up via block height)

cleanup

7b3b5d6

better api support for block outputs with range proofs

90ad055

antiochp added 3 commits January 14, 2018 21:25

introduce OutputIdentifier, avoid leaking SumCommit everywhere

baaf799

Merge branch 'master' into output_pmmr_hash

51be869

fix the bad merge

00c0537

antiochp mentioned this pull request Jan 15, 2018

[WIP] coinbase outputs with lock_height in switch commitments #215

Closed

13 tasks

pool verifies coinbase maturity via is_matured

bc4364a

this uses sumtree in a consistent way

ignopeverell reviewed Jan 16, 2018

View reviewed changes

antiochp added 8 commits January 16, 2018 07:45

cleanup

3fe7f5c

add docs, cleanup build warnings

61b30e6

fix core tests

8928bca

fix chain tests

c698673

fix pool tests

aa3b739

Merge branch 'master' into output_pmmr_hash

4b80da6

cleanup debug logging that we no longer need

d6b5413

make out_block optional on an input (only care about it for spending …

ea291d9

…coinbase outputs)

antiochp changed the title ~~[WIP] hash (features|commitment) in output mmr~~ hash (features|commitment) in output mmr Jan 16, 2018

cleanup

90a3755

bump the build

adc155b

antiochp merged commit cbd3b2f into mimblewimble:master Jan 17, 2018

antiochp deleted the output_pmmr_hash branch January 17, 2018 03:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

hash (features|commitment) in output mmr #615

hash (features|commitment) in output mmr #615

antiochp commented Jan 15, 2018 •

edited

Loading

antiochp commented Jan 16, 2018

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

ignopeverell commented Jan 16, 2018

antiochp commented Jan 16, 2018

ignopeverell commented Jan 16, 2018

antiochp commented Jan 16, 2018 •

edited

Loading

antiochp commented Jan 17, 2018

		@@ -176,6 +183,23 @@ fn test_coinbase_maturity() {

		let prev = chain.head_header().unwrap();

		<<<<<<< HEAD

hash (features|commitment) in output mmr #615

hash (features|commitment) in output mmr #615

Conversation

antiochp commented Jan 15, 2018 • edited Loading

antiochp commented Jan 16, 2018

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

ignopeverell commented Jan 16, 2018

antiochp commented Jan 16, 2018

ignopeverell commented Jan 16, 2018

antiochp commented Jan 16, 2018 • edited Loading

antiochp commented Jan 17, 2018

antiochp commented Jan 15, 2018 •

edited

Loading

antiochp commented Jan 16, 2018 •

edited

Loading