Ensure new coins can't be spent for X blocks about grin HOT 20 CLOSED

First part of this implemented in 38d5d67, by explicitly flagging coinbase outputs.

from grin.

Question about this -
I see we have OutputFeatures (for txn outputs) and KernelFeatures (for txn kernels), identifying an output and a transacton that spends a coinbase.
What stops these bits being modified or altered during use? If a miner simply flipped the "this is a coinbase" bit then would this not defeat any attempt at preventing it from being spent?

Would signing the features alongside the fee on a transaction eliminate this?

from grin.

A miner can't mess with these bits without invalidating the coinbase sums of outputs and kernels:

https://github.com/ignopeverell/grin/blob/master/core/src/core/block.rs#L418

If you want to convince yourself of it, adding a unit test would be great :-)

from grin.

I see some discussion around a possibly related idea in #25?

Feels like these two requirements "prevent spending of coinbase for n blocks" and "time locked transactions" may be examples of a more generalized solution somehow - like tracking a "minimum block height before spendable" on outputs of a transaction. Maybe every transaction has this (and normally this is the current block height)?

from grin.

Ah got it - on the block containing the coinbase itself we check that the coinbase output (presumably a single output normally?) and the coinbase transaction kernels are all consistent.

I think my question was poorly written - I'm thinking more in terms of what happens when that coinbase output becomes an input into a subsequent transaction, say in the next block vs. 1001 blocks later.
My initial rough idea was to track the features on the input itself, based on the features from the underlying output. There would be nothing to prevent these bits from being modified though or am I missing something?

(And yes a unit test here is something I'd definitely like to put together to convince myself of some of this...)

from grin.

No, you have to track the features on the output the input is referencing. If the output is a coinbase, then you check the block distance.

from grin.

Throwaway spike/experiment PR up here - antiochp#1
@ignopeverell wondering if you had thoughts on what I am attempting here.

This is more playing around to get a better feel for the various data structures in grin than a real attempt at solving this issue but I would appreciate any feedback you have?

from grin.

This looks like it's going the right way. As your comment indicates, it needs to be merged with get_unspent somehow as the current brute-force lookup block by block and output by output definitely won't perform very well. We may need to tweak the storage structure a little to have that information readily available.

Also, not sure if you haven't come around it yet, but similar validation will need to be done on block outputs.

from grin.

In terms of the storage structure - are we opposed to swapping out the Vec<Input> and Vec<Output> and using HashMap<Commitment, Input> and HashMap<Commitment, Output> (on both Block and Transaction)?
This may not be the ideal long term solution but medium term it will make the lookups by commitment more reasonable.

from grin.

I was thinking about database storage structure. Loading all blocks backward from storage is still going to be a lot more expensive than an in-memory iteration. Maybe commitment to block header hash entries in rocksdb?

from grin.

Realizing how steep the learning curve is here...
Now I get what self.store.get_output_by_commit() and self.store.get_block() are doing (they are pulling data from rocksdb and deserializing it).

Planning to take some time to read up on the store code and rocksdb in general but I think I understand what you are suggesting -
effectively building an "index" in rocksdb commitment -> block header hash so we can then pull the block header directly without having to traverse the full chain block header hash -> block header.

Exposed through something roughly like -

• self.store.get_block_header_by_output_commit()
• self.store.get_block_header_by_input_commit()

I assume we will eventually need to handle a re-org (rewound?) but will worry about that later.

from grin.

Correct on everything.

from grin.

PR up for adding (and using) get_block_header_by_output_commitment() - #104 (for discussion)

from grin.

Reworked the previous spike based on better understanding - ignopeverell/grin#111
Any additional feedback would be much appreciated.

I think this covers this rule when adding new transactions via add_to_memory_pool.
I think validate_block in pipe.rs is where I need to be looking to do the same thing for blocks themselves - let me know if my thinking is wrong here.

Also need to make sure we have decent test coverage around this.

from grin.

@ignopeverell
Couple of questions about outputs and commitments -

The block reward is currently hard-coded to 1_000_000.
For a given miner reward_key - every coinbase output for every block mined will have the same commitment - is my understanding correct here?
i.e. if a miner reuses a private key then an identical commitment is generated?

This has surfaced in a couple of places while adding test coverage for #111 -

1. in block::compact() we appeared to be actually compacting away coinbase outputs and inputs if we attempted to spend a coinbase output (the input commitment matched the previous output commitment)
   I believe I have fixed this by excluding coinbase inputs/outputs from the compact logic.
   See https://github.com/ignopeverell/grin/pull/111/files#diff-dd827c758488146ce2a4f07e8bab6a26R324

2. This is the bigger issue I think - if every coinbase output for every block mined against a given reward_key has an identical commitment - we have no way of knowing how old a coinbase output is (we index the block header in the store by commitment).
   Each new coinbase output effectively overwrites the previous one in the index (and it never gets old enough to mature as far as the current logic is concerned).

Edit:
This line in the whitepaper looks to be related -

We make the further restriction on transactions that every input within a transaction be unique.

I sort of see why this makes sense for an individual transaction. But if the inputs and outputs of a block can be treated as a single transaction for validation purposes - does this restriction also apply? And if we can apply cut-through across multiple blocks - does it also apply here?

Is the answer here to simply never reuse a private key - and to enforce this somehow?
Is there a way to guarantee this?

from grin.

Yes, we really want to enforce each commitment to be unique. Otherwise this can create a few headaches, like the ones you mention. And we generally don't want people to reuse keys for even more reasons than in Bitcoin.

from grin.

So in the tests do we just want to use a unique reward_key for each mined block and assume this is being enforced elsewhere, at least for now?

from grin.

Also looking through the code - I see check_duplicate_outputs is used to actually enforce this from the transaction point of view.
So maybe we need to do something similar with coinbase outputs on a block itself - as I don't think this is prevented currently for coinbase outputs.

from grin.

Yes, just assume it's in there for now. It will actually be enforced by what I'm working on right now: the UTXO sum tree.

from grin.

This should be resolved by #111

from grin.