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

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

Closed
wants to merge 38 commits into from

Conversation

antiochp
Copy link
Member

@antiochp antiochp commented Oct 27, 2017

TODO -

  • store switch_commit_hash in output_pmmr (DONE maintain switch_commit_hashes in the output pmmr (not used yet) #583)
  • remove header_by_output_commit index
  • remove output_by_commit index
  • input needs to provide switch_commit + lock_height
  • sumtree is_unspent/get_unspent return switch_commit_hash (all we have)
  • rethink outputs vs utxos in api (we know a lot less about utxos now, sumtree only)
    • we do not even know if an output is a coinbase or not from the info in the sumtrees...
  • wallet restore and refresh/checker needs rethinking
    • no easy access to heights based on commitments etc.
    • no easy access to output type (coinbase or not coinbase)
    • but - if we retrieve outputs based on a block height/hash then we do have this info
    • no access to rangeproof (to recover value) unless we go via the block height/hash
  • we discussed merkle proofs in gitter - tackle this in separate PR?
    • I don't think this is a blocker for this work and solves a related but not identical issue around ensuring we are spending the output we think we are spending.
    • tackling this in separate PR - assumption made that we have full blocks (+ outputs) in the db (issue TBD)
  • review recent conversation in mailing list re switch commits
  • fix failing tests
  • fix issue with restore/refresh (reused wallet key for coinbase output)
  • review
  • Q) can we skip the switch_commit_hash check for non-coinbase?
    • is there a way to trust coinbase/non-coinbase without switch_commit_hash?

Old info (needs revisiting).

Coinbase outputs use (output_features || lock_height) as the secret key to the blake2 hash in the switch commitment hash.
This allows us to timelock coinbase outputs in a self-contained way (does not rely on retrieving the originating block header for the block that generated the coinbase output).
The height h must be revealed to spend the coinbase output.

Normal tx outputs use zero array as the secret key - this way we can still leverage the switch commitment hash to identify out own outputs when recovering a wallet.

  • switch commitment: rJ
  • switch commit hash: blake2(rJ)
  • proposed switch commit hash: blake2(h, rJ) where h the lock_height is used as the secret key to the hash function.

Proposal: to spend an output timelocked with a lock_height (currently only applies to coinbase outputs) you need to reveal rJ and h.

We can verify an input based on the switch commitment and lock_height from the output.
The hash function ensures nobody is lying about the lock_height as the switch commit hash needs to match the one on the output itself.

This assumes every verifier has the output - this is fine for coinbase outputs.
I don't believe we know how to do this for the general case due to possible cut-through of outputs.

@apoelstra
Copy link

I'm pretty sure there's no way you can put any restriction on outputs that isn't somehow hidden inside the Pedersen commitments (e.g. you can do multisig this way). There is nothing in MW signing the exact output, except perhaps some p2p-level things to prevent noninteractive merging that would then require miner cooperation to work around, so the recipient is free to replace it with anything else that adds up to the same Pedersen commitment.

Cut-through is just one instance of this, where the recipient makes another transaction spending the would-be locktimed output.

Typical uses of locktimed outputs involve the creation of refund transactions, where the original output isn't even broadcast until the refund transaction has been created. In this situation, by sending both the funding transaction and the refund transaction to a malicious miner, somebody can completely override the locktime in a non-consensus-detectable way.

@antiochp antiochp force-pushed the lockheight_outputs branch 4 times, most recently from 31088a1 to d49cb3b Compare November 2, 2017 19:40
@antiochp antiochp changed the title [DNM] experiment with lock_heights on outputs coinbase outputs with lock_height in switch commitments Nov 13, 2017
@antiochp antiochp changed the title coinbase outputs with lock_height in switch commitments [DNM] coinbase outputs with lock_height in switch commitments Nov 13, 2017
@antiochp antiochp changed the title [DNM] coinbase outputs with lock_height in switch commitments coinbase outputs with lock_height in switch commitments Nov 13, 2017
@ignopeverell
Copy link
Contributor

Is this ready for review now (wish github would tell me when you added reviewers to the PR)?

@antiochp
Copy link
Member Author

@ignopeverell I think so yes.

I'm not 100% convinced this is the right approach right now but I'm keen on getting your feedback.

We're increasing the size of each Input to cover the "revealed" switch commit and lock_height.
These only get used for inputs spending coinbase (effectively ignored for regular txs).
I think it is a step toward a more generalized solution (although we don't know this as we can't do conditional outputs yet).

I guess the question is - is the increased complexity and larger inputs worth it to allow us to encode lock_heights in coinbase outputs like this?

@ignopeverell
Copy link
Contributor

I haven't forgotten this, but I'm still pondering.

@antiochp
Copy link
Member Author

No rush - I'm totally happy to close this for now and we can revisit later if that makes more sense.

@antiochp antiochp force-pushed the lockheight_outputs branch from 439dca9 to ad47d12 Compare January 3, 2018 15:35
@antiochp antiochp changed the title coinbase outputs with lock_height in switch commitments [WIP] coinbase outputs with lock_height in switch commitments Jan 3, 2018
@antiochp
Copy link
Member Author

antiochp commented Jan 5, 2018

Alternative approach to "coinbase maturity validation needs to to be fork aware" here - #580, maintain heights in a sumtree pmmr, keep lock_heights out of the inputs/outputs themselves.

@antiochp antiochp force-pushed the lockheight_outputs branch from c369a0e to 94f70f5 Compare January 7, 2018 16:01
switch commit hash key encodes lock_height
cleanup output by commit index (currently broken...)
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)
@antiochp
Copy link
Member Author

antiochp commented Jan 8, 2018

Going to be saving a fair bit of disk space like this if we're no longer storing outputs (+ rangeproofs) in the index.
They are stored in the full blocks but that's the only copy of the rangeproofs in the db.

@antiochp
Copy link
Member Author

antiochp commented Jan 9, 2018

@ignopeverell - this is ready for review/discussion.
Got all the tests passing.
Got wallet working (refresh and restore).


hashable_ord!(Input);

/// Implementation of Writeable for a transaction Input, defines how to write
/// an Input as binary.
impl Writeable for Input {
fn write<W: Writer>(&self, writer: &mut W) -> Result<(), ser::Error> {
writer.write_fixed_bytes(&self.0)
writer.write_fixed_bytes(&self.commit)?;
writer.write_fixed_bytes(&self.switch_commit)?;

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

SwitchCommitHashKey::from_lock_height(self.lock_height),
);

if switch_commit_hash != *output_switch_commit_hash {

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

…et keys)

for coinbase outputs where lock_height is _very_ important
@antiochp
Copy link
Member Author

antiochp commented Jan 10, 2018

Writing down what I understand to be the proposed approach here based on feedback -

We modify input ser/deser to -

  • include output_features (coinbase vs non-coinbase spend)
  • conditionally include switch_commit_hash (only for coinbase spend, keep non-coinbase small)

Then on tx pool and block validation -
we only check lock_height (via switch_commit_hash) for inputs flagged as spending coinbase output.

  1. I think we would then require the Merkle Proof approach to be able to fully verify lock_height by preventing miner lying about original height. merkle_proof -> block_hash -> block_height (or something similar) and can validate lock_height - height etc. and be sure we are not being lied to.

  2. But what prevents a miner lying about spending coinbase vs. non-coinbase?
    If we cannot rely on full blocks or ability to lookup outputs themselves.
    And we only have what's in the sumtrees for validation?
    We don't have any way of validating if an output is coinbase or not (that I can see).

We can implement (1) as a separate PR and build on what we have done in this PR.
And for now assume the miner is honest?
Alternatively we know the block hash (the miner generates it) - so we could require the block_hash to be included as part of an input (coinbase only?) and assume a full archive node for now (look the block up in the db and find the output, check if it coinbase or not).

I'm concerned about (2) though - is my thinking correct here or am I missing something simple? There is nothing in the sumtree that shows an output is coinbase or not.

@ignopeverell
Copy link
Contributor

ignopeverell commented Jan 10, 2018

Overall I agree but I have a couple remarks that may make things easier:

  • As what's being spent is (normally) a bona fide UTXO, we'll always have the whole output data.
  • For now, as we only have a full node, we can rely on having the full block. We can tackle the "full block isn't needed" part with the merkle proof separately.

@antiochp
Copy link
Member Author

Closing - replaced with #615.

@antiochp antiochp closed this Jan 15, 2018
@antiochp antiochp deleted the lockheight_outputs branch May 2, 2018 11:30
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.

3 participants