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.

Sign up for GitHub

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

Jump to bottom

Cannot spend coinbase for N blocks #111

Merged
merged 14 commits into from Sep 12, 2017
Merged

Cannot spend coinbase for N blocks #111

merged 14 commits into from Sep 12, 2017

Conversation

antiochp
Copy link
Member

@antiochp antiochp commented Aug 30, 2017 •
edited

  • introduce core errors in core/types.rs - this is WIP (not sure where to put these)
  • map errors in adapters (chain error -> core error)
  • use get_block_header_by_output_commit and head_header in add_to_memory_pool
  • introduce COINBASE_MATURITY const in consensus (currently 10 for testing/dev)
  • introduce PoolError::ImmatureCoinbase
  • return output in Parent::BlockTransaction
  • add same coinbase maturity check to validate_block
  • test process_block with unique reward_key per block (not enforced but by convention for it to work correctly)

TODO -

  • add similar consensus check when adding a new block - validate_block() I think?
  • confirm we are not off-by-1 on block validation vs. txn validation rules
  • test coverage around add_to_memory_pool to cover ImmatureCoinbase path
  • test coverage around validate_block to cover ImmatureCoinbase path
  • we do not currently prevent "duplicate" coinbase outputs (to be revisited)
  • reduced the COINBASE_MATURITY down from 1_000 to 10 for testing (revisit later)

@antiochp antiochp mentioned this pull request Aug 30, 2017
Closed
4 tasks
@antiochp antiochp changed the title Coinbase spent n blocks Cannot spend coinbase for N blocks Aug 30, 2017
@antiochp antiochp mentioned this pull request Aug 30, 2017
Closed
ignopeverell
ignopeverell reviewed Aug 30, 2017
View reviewed changes
Copy link
Contributor

@ignopeverell ignopeverell left a comment

Choose a reason for hiding this comment

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

First set of comments and minor remarks. Directionally it looks good.

chain/src/chain.rs Outdated
self.store.get_block_header_by_output_commit(commit).map_err(
&Error::StoreErr,
)
pub fn get_block_header_by_output_commit(&self, commit: &Commitment) -> Option<BlockHeader> {

This comment was marked as spam.

Sign in to view

chain/src/pipe.rs Outdated
// TODO check every input exists as a UTXO using the UTXO index

// now check that any coinbase inputs have matured sufficiently
if let Ok(output) = ctx.store.get_output_by_commit(&input.commitment()) {

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

chain/src/types.rs Outdated
@@ -57,6 +57,8 @@ pub enum Error {
InvalidBlockTime,
/// Block height is invalid (not previous + 1)
InvalidBlockHeight,
/// coinbase can only be spent after it has matured (n blocks)
ImmatureCoinbase,

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

grin/src/adapters.rs Outdated
@@ -295,4 +296,12 @@ impl pool::BlockChain for PoolToChainAdapter {
fn get_unspent(&self, output_ref: &Commitment) -> Option<Output> {
self.chain.borrow().get_unspent(output_ref)
}

fn get_block_header_by_output_commit(&self, commit: &Commitment) -> Option<BlockHeader> {

This comment was marked as spam.

Sign in to view

antiochp
antiochp commented Aug 31, 2017
View reviewed changes
core/src/types.rs Outdated
#[derive(Debug)]
pub enum CoreError {
/// TODO - rename this
GenericCoreError,

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

antiochp
antiochp commented Sep 1, 2017
View reviewed changes
pool/src/types.rs
/// TODO - wip, just getting imports working, remove this and use more specific errors
GenericPoolError,
/// TODO - is this the right level of abstraction for pool errors?
OutputNotFound,

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@antiochp antiochp changed the title Cannot spend coinbase for N blocks [WIP] Cannot spend coinbase for N blocks Sep 1, 2017
@antiochp antiochp force-pushed the coinbase_spent_n_blocks branch 2 times, most recently from f8a7256 to c9ed989 Compare September 5, 2017 20:03
antiochp
antiochp commented Sep 6, 2017
View reviewed changes
core/src/core/block.rs

let out_set = self.outputs
.iter()
.filter(|out| !out.features.contains(COINBASE_OUTPUT))

This comment was marked as spam.

Sign in to view

antiochp
antiochp commented Sep 6, 2017
View reviewed changes
core/src/core/block.rs
@@ -429,12 +438,12 @@ impl Block {
fn verify_coinbase(&self, secp: &Secp256k1) -> Result<(), secp::Error> {
let cb_outs = self.outputs
.iter()
.filter(|out| out.features.intersects(COINBASE_OUTPUT))
.filter(|out| out.features.contains(COINBASE_OUTPUT))

This comment was marked as spam.

Sign in to view

@antiochp
Copy link
Member Author

antiochp commented Sep 6, 2017

@ignopeverell thoughts/feedback on how this is progressing?
What's outstanding from your perspective to get this finished?

Went into this thinking this would be a relatively minor change... definitely given me a good deep-dive into the existing txn and block processing code.

@antiochp
wip
000756d
@antiochp
make sure it compiles
c5b0ded
@antiochp
use head_header in add_to_memory_pool
6fa37c7
@antiochp
add COINBASE_MATURITY const to consensus
fbe2874
@antiochp
add coinbase maturity (wip) validaton rule to validate_block
981214c
@antiochp
add coinbase maturity check to validate_block
bc46490
@antiochp
fix up tabs/spaces
7e39afc
@antiochp
map errors in adapters - specific errors still wip
5436666
@antiochp
reworked so adapter translates chain errors to pool errors (core erro…
8ff8309 
…rs not required)
@antiochp
add test for spending immature coinbase in memory pool
3b03c2e
@antiochp
wip - add test to cover spending coinbase output in chain.process_block
41cd6d6
@antiochp
added test coverege around process_block - we have a problem with coi…
366344a 
…nbase output commitments
@antiochp
add a comment on the failing test
764eecd
@antiochp
process_block will now fail validation if we attempt to spend coinbas…
1456a17 
…e that has not yet matured

(remember to use a new reward_key for every block)
test coverage in place to verify this
@ignopeverell
Copy link
Contributor

So what's the status on this? Are we giving it a little time to see if a solution that's also applicable to time locks can be found?

@antiochp antiochp changed the title [WIP] Cannot spend coinbase for N blocks Cannot spend coinbase for N blocks Sep 10, 2017
@antiochp
Copy link
Member Author

@ignopeverell I think this is good to go. As you mentioned in another thread we should be able to migrate this to a more generalized solution for time locks. But for coinbase specific locks I think this works pretty well.
Thoughts/feedback?

@ignopeverell ignopeverell merged commit 95a92ee into mimblewimble:master Sep 12, 2017
@antiochp antiochp deleted the coinbase_spent_n_blocks branch September 12, 2017 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ignopeverell ignopeverell ignopeverell left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@antiochp @ignopeverell