Skip to content

Conversation

@random-zebra
Copy link

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

Based on top of:

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.

@random-zebra random-zebra self-assigned this Nov 12, 2020
@random-zebra random-zebra force-pushed the 202011_test-mempool-sapling branch 3 times, most recently from 1b823b2 to 7d716b2 Compare November 13, 2020 13:18
verify that txes double-spending a nullifier are not accepted in the
mempool, and that txes referencing a disconnected anchor are removed.
And run that list in Travis job n.8 along with tiertwo tests.
@random-zebra random-zebra added this to the 5.0.0 milestone Nov 14, 2020
@random-zebra random-zebra force-pushed the 202011_test-mempool-sapling branch from 7d716b2 to 60ea1d8 Compare November 14, 2020 14:32
@random-zebra
Copy link
Author

Rebased on master. Ready for review.
Also added a commit to move the sapling tests to the 8th Travis job (together with the tiertwo tests), as the main test_runner job, due to the high number of tests added (...which is awesome 😎 ) is starting to consistently hit the 50 minutes timeout.

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.

Pretty nice!, code review ACK 60ea1d8

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 60ea1d8

@Fuzzbawls Fuzzbawls merged commit 56b133f into PIVX-Project:master Nov 15, 2020
@Fuzzbawls Fuzzbawls added the Needs Release Notes Placeholder tag for anything needing mention in the "Notable Changes" section of release notes label Dec 14, 2020
@random-zebra random-zebra removed the Needs Release Notes Placeholder tag for anything needing mention in the "Notable Changes" section of release notes label Jan 3, 2021
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.

3 participants