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

[Validation] Sapling nullifiers mempool connection #1958

Merged
merged 10 commits into from
Nov 12, 2020

Conversation

random-zebra
Copy link

@random-zebra random-zebra commented Nov 9, 2020

Keep track of nullifiers of notes spent by mempool transactions (preventing in-mempool double spends).
Add consistency checks with coins view nullifiers.

Also, if the sapling anchor changes after a disconnection, we must evict from mempool any transaction that spends from the now-invalid root.

Built on top of:

@random-zebra
Copy link
Author

Rebased.

Copy link

@furszy furszy left a comment

Choose a reason for hiding this comment

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

nice work, code review ACK 93e308e.
Only minor comments for now.

src/txmempool.cpp Outdated Show resolved Hide resolved
src/txmempool.cpp Outdated Show resolved Hide resolved
@random-zebra
Copy link
Author

Nits addressed.

Copy link
Collaborator

@Fuzzbawls Fuzzbawls left a comment

Choose a reason for hiding this comment

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

utACK 7e2d5a6

Copy link

@furszy furszy left a comment

Choose a reason for hiding this comment

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

ACK 7e2d5a6

@furszy furszy merged commit efa4a94 into PIVX-Project:master Nov 12, 2020
Fuzzbawls added a commit that referenced this pull request Nov 15, 2020
…chor

60ea1d8 [CI][Tests] Move sapling functional tests to their own list (random-zebra)
03aec32 [Tests] Add sapling_mempool to test_runner and re-sort the list (random-zebra)
c3a1fff [Test] Add sapling_mempool functional test (random-zebra)
101978d [RPC] Introduce getbestsaplinganchor to get the most recent sapling root (random-zebra)

Pull request description:

  Based on top of:
  - [x] #1958
  - [x] #1964

  This adds a functional test to check the bahavior of sapling transactions with the mempool.
  Specifically, it verifies the nullifiers/anchor management introduced in #1958:
  - If a transaction spends a sapling note, already spent by another in-mempool transaction, it's not accepted in the mempool.
  - Same if it tries to double-spend a note already spent on chain.
  - If, after disconnecting a block, the sapling merkle tree root changes, any transaction that spends a note referencing the old root must be evicted from the mempool.

  In order to verify the last point, a simple RPC `getbestsaplinganchor` is introduced in the "blockchain" category.
  It returns the `pcoinsTip` best anchor.

  Note: without 1958, the test crashes the node, as two transactions spending the same note can both enter the mempool, but then the assertions in the memory pool consistency checks fail during block creation.

ACKs for top commit:
  furszy:
    Pretty nice!, code review ACK 60ea1d8
  Fuzzbawls:
    utACK 60ea1d8

Tree-SHA512: c321d7866bec56fca31d48286ac28ae44f27c48232919fd3f66fcc858a558b17cc0580163790c74290f376049fa44391a1033eb582ff9716b502673f535048d6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants