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

fix(consensus): Verify the lock times of mempool transactions #6027

Merged
merged 5 commits into from Jan 27, 2023

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Jan 24, 2023

Motivation

We need to check that mempool transaction lock times are valid, so the block templates created by Zebra are also valid.

Closes #5984

Specifications

Mempool Transaction Verification

Define the median-time-past of a block to be the median (as defined in § 7.7.3 ‘Difficulty adjustment’ on p. 131) of the nTime fields of the preceding PoWMedianBlockSpan blocks (or all preceding blocks if there are fewer than PoWMedianBlockSpan). The median-time-past of a genesis block is not defined.

• As in Bitcoin, the nTime field MUST represent a time strictly greater than the median of the timestamps of the past PoWMedianBlockSpan blocks.

https://zips.z.cash/protocol/protocol.pdf#blockheader

If greater than or equal to 500 million, locktime is parsed using the Unix epoch time format (the number of seconds elapsed since 1970-01-01T00:00 UTC—currently over 1.395 billion). The transaction can be added to any block whose block time is greater than the locktime.

https://developer.bitcoin.org/devguide/transactions.html#locktime-and-sequence-number

If the transaction's lock time is less than the median-time-past, it will always be less than the next block's time, because the next block's time is strictly greater than the median-time-past.

This is the rule implemented by zcashd:
https://github.com/zcash/zcash/blob/9e1efad2d13dca5ee094a38e6aa25b0f2464da94/src/main.cpp#L776-L784
https://github.com/zcash/zcash/blob/9e1efad2d13dca5ee094a38e6aa25b0f2464da94/src/main.cpp#L735

Mempool Updates on State Tip Changes

When a new block is added to the state tip:

  • existing transactions in the mempool will always have valid lock times, because the block time is strictly increasing
  • previously rejected lock times might have become valid, but the mempool already clears all verifier errors on every new block

Complex Code or Requirements

This PR modifies async code, and state code that runs concurrently with block writes.

Solution

  • Implement the BestChainNextMedianTimePast state request
  • Verify the lock times of mempool transactions
  • Document that the mempool already handles lock time rejections correctly

This is a mempool consensus rule bug, so we don't want these changes behind a feature flag.

Testing

  • Fix existing tests to have valid lock times
  • Add new tests for each lock time consensus rule: unlocked, sequence numbers, past lock time, before lock time

Review

I marked this as a high priority because it is blocking most of our testing work.
Because it is a high priority, I'd like to avoid doing refactors in this PR.

Reviewer Checklist

  • Will the PR name make sense to users?
    • Does it need extra CHANGELOG info? (new features, breaking changes, large changes)
  • Are the PR labels correct?
  • Does the code do what the ticket and PR says?
    • Does it change concurrent code, unsafe code, or consensus rules?
  • How do you know it works? Does it have tests?

@teor2345 teor2345 added C-bug Category: This is a bug A-consensus Area: Consensus rule updates P-High 🔥 I-consensus Zebra breaks a Zcash consensus rule A-state Area: State / database changes I-lose-funds Zebra loses user funds A-concurrency Area: Async code, needs extra work to make it work properly. labels Jan 24, 2023
@teor2345 teor2345 self-assigned this Jan 24, 2023
@github-actions github-actions bot added the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Jan 24, 2023
@teor2345 teor2345 marked this pull request as ready for review January 24, 2023 22:45
@teor2345 teor2345 requested review from a team as code owners January 24, 2023 22:45
@teor2345 teor2345 requested review from upbqdn, arya2 and oxarbitrage and removed request for a team January 24, 2023 22:45
@teor2345
Copy link
Contributor Author

This PR needs 2 reviewers, so I've tagged everyone else who is working on Zebra for a review.

If you're the second reviewer, you can take the third person off!

@codecov
Copy link

codecov bot commented Jan 25, 2023

Codecov Report

Merging #6027 (0af4c1a) into main (69a64b6) will decrease coverage by 0.12%.
The diff coverage is 67.60%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6027      +/-   ##
==========================================
- Coverage   78.04%   77.92%   -0.12%     
==========================================
  Files         312      312              
  Lines       39036    39418     +382     
==========================================
+ Hits        30464    30717     +253     
- Misses       8572     8701     +129     

Copy link
Contributor

@arya2 arya2 left a comment

Choose a reason for hiding this comment

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

This looks good to me, it should work as-is but I left a few optional comments for clean up.

zebra-state/src/service/read/find.rs Show resolved Hide resolved
zebra-state/src/service/read/find.rs Show resolved Hide resolved
zebra-state/src/service/read/find.rs Show resolved Hide resolved
zebra-consensus/src/transaction.rs Show resolved Hide resolved
zebra-consensus/src/transaction/tests.rs Show resolved Hide resolved
@teor2345 teor2345 requested a review from arya2 January 26, 2023 19:17
Copy link
Contributor

@arya2 arya2 left a comment

Choose a reason for hiding this comment

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

Looks good, thank you for the fix!

I left a do-not-merge label in case anyone else is currently reviewing this, feel free to remove it when you want this to embark on the merge train.

@arya2 arya2 added the do-not-merge Tells Mergify not to merge this PR label Jan 26, 2023
@teor2345
Copy link
Contributor Author

I would like two reviews, because this is consensus-critical concurrent code.

Copy link
Contributor

@oxarbitrage oxarbitrage left a comment

Choose a reason for hiding this comment

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

looks good to me.

@arya2 arya2 removed the do-not-merge Tells Mergify not to merge this PR label Jan 27, 2023
mergify bot added a commit that referenced this pull request Jan 27, 2023
@mergify mergify bot merged commit e20cf95 into main Jan 27, 2023
@mergify mergify bot deleted the check-mempool-lock-time branch January 27, 2023 21:46
@oxarbitrage oxarbitrage mentioned this pull request Jan 29, 2023
36 tasks
teor2345 added a commit that referenced this pull request Feb 6, 2023
… - manually delete getblocktemplate-rpcs

* Implement the BestChainNextMedianTimePast state request

* Verify the lock times of mempool transactions

* Document that the mempool already handles lock time rejections correctly

* Fix existing tests

* Add new mempool lock time success and failure tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-concurrency Area: Async code, needs extra work to make it work properly. A-consensus Area: Consensus rule updates A-state Area: State / database changes C-bug Category: This is a bug C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG I-consensus Zebra breaks a Zcash consensus rule I-lose-funds Zebra loses user funds
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Transaction nLockTIme check (CheckFinalTx) against MTP when adding to mempool possible missed
3 participants