Optimize MerkleTree.contains from O(n) to O(1) #775

leanthebean · 2021-12-17T04:53:53Z

Summary

In the merkle tree file, we have a contained method. Currently, there's a linear scan that goes through all of the existing nullifiers when determining if a transaction is valid (e.g. not a double spend). As we get more transactions, this linear scan becomes more and more inefficient (e.g. beefy later blocks would take several seconds).

This change adds another db store that lets you check in O(1) if an element exists in a merkle tree.

Testing Plan

Tested it locally and avg block time is about 20ms per transaction per block

Breaking Change

This requires a db upgrade.

Yes
No

dguenther

From a code review standpoint, looks good -- I think @NullSoldier wants to add a perf test to this before it merges, and it'd be nice to run it on a laptop node for a bit to see if any issues happen to pop up.

dguenther · 2021-12-18T01:15:01Z

ironfish/src/consensus/verifier.ts

@@ -167,10 +167,8 @@ export class Verifier {
    tx?: IDatabaseTransaction,
  ): Promise<VerificationResult> {
    return this.chain.db.withTransaction(tx, async (tx) => {
-      const noteSize = await this.chain.notes.size(tx)


Ah, looks like this might have been a bug, good spot! If possible it might be nice to pull this out into a separate PR so any issues with it aren't conflated with the LeavesIndex cache.

Moved that fix here, #798

dguenther · 2021-12-18T01:41:26Z

It looks like there are some test failures in here as well

NullSoldier · 2021-12-21T05:56:40Z

Here is the perf test @dguenther https://github.com/iron-fish/ironfish/pull/799/files

ironfish/src/merkletree/merkletree.ts

NullSoldier · 2021-12-21T06:15:10Z

ironfish/src/consensus/verifier.ts

@@ -296,16 +294,15 @@ export class Verifier {
  async hasValidSpends(block: Block, tx?: IDatabaseTransaction): Promise<VerificationResult> {


The removed code that was buggy here was intending to check for the note being spent really existed in the tree at the time it was spent but was not. Should this still check for that some how? The spend size is the size of the nullifier tree when the commitment was created, not the note tree right? @dguenther

had a horribly inefficient linear scan which brought up avg time to add a block with multiple transactions to multiple seconds making syncing painfully slow. This change introduces another db store to keep track of merkle tree elements for O(1) retrieval if the element is in the tree or not.

NullSoldier · 2021-12-22T07:22:41Z

We still need to clean up the index in the MerkleTree.truncate function.

dguenther · 2021-12-22T21:10:19Z

ironfish/src/consensus/verifier.ts

@@ -327,22 +331,21 @@ export class Verifier {
   * spend's spend root.
   *
   * @param spend the spend to be verified
-   * @param size the size of the nullifiers tree at which the spend must not exist


This should probably be added back, or we should kill the whole comment so it's at least somewhat consistent

* In merkletree's contained & contains method we had a horribly inefficient linear scan which brought up avg time to add a block with multiple transactions to multiple seconds making syncing painfully slow. This change introduces another db store to keep track of merkle tree elements for O(1) retrieval if the element is in the tree or not. * removing pastSize from the contained method * Use existing merkle hash * Fix dead lock * Minor cleanup inside of verifier (#811) * Rename hasValidSpends to verifyConnectedSpends (#809) * Only verify spend root in hasValidSpends (#810) * Truncate index during MerkleTree truncate (#820) Co-authored-by: NullSoldier <nullprogrammer@gmail.com>

* Use nullifier tree size to verify spend (#798) We were using the notes tree size to verifier the spend, which is not correct but shouldn't generally cause an issue because the note tree is bigger than the nullifier tree. * Fix use of clearLine in miners:mined (#816) * chore: add graffiti logging on start (#818) * Set default expiration delta to 15 from 450 (#822) * Use @napi-rs/blake-hash instead of blake3 (#815) The blake3 package doesn't seem to ship a native addon that works properly in node 16, so let's try blake-hash instead. * Optimize MerkleTree.contains from O(n) to O(1) (#775) * In merkletree's contained & contains method we had a horribly inefficient linear scan which brought up avg time to add a block with multiple transactions to multiple seconds making syncing painfully slow. This change introduces another db store to keep track of merkle tree elements for O(1) retrieval if the element is in the tree or not. * removing pastSize from the contained method * Use existing merkle hash * Fix dead lock * Minor cleanup inside of verifier (#811) * Rename hasValidSpends to verifyConnectedSpends (#809) * Only verify spend root in hasValidSpends (#810) * Truncate index during MerkleTree truncate (#820) Co-authored-by: NullSoldier <nullprogrammer@gmail.com> * Bump version to 0.1.15 * Update docker-compose configurations * Fix path and change services name * Fix readme * Add volume to miner service * Proofreading Docker.md Co-authored-by: Jason Spafford <nullprogrammer@gmail.com> Co-authored-by: Derek Guenther <derek@ironfish.network> Co-authored-by: jintao <cdv1013@gmail.com> Co-authored-by: Elena Nadolinski <elena@ironfish.network> Co-authored-by: Rohan Jadvani <5459049+rohanjadvani@users.noreply.github.com> Co-authored-by: Derek Guenther <dguenther9@gmail.com>

leanthebean requested a review from a team as a code owner December 17, 2021 04:53

leanthebean changed the base branch from master to staging December 17, 2021 04:54

dguenther approved these changes Dec 18, 2021

View reviewed changes

dguenther force-pushed the staging branch 4 times, most recently from a342cae to 8474295 Compare December 18, 2021 04:19

NullSoldier changed the title ~~Merkle tree contains~~ Optimize MerkleTree.contains to not linear scan Dec 21, 2021

NullSoldier changed the title ~~Optimize MerkleTree.contains to not linear scan~~ Optimize MerkleTree.contains from O(n) to O(1) Dec 21, 2021

NullSoldier reviewed Dec 21, 2021

View reviewed changes

ironfish/src/merkletree/merkletree.ts Outdated Show resolved Hide resolved

NullSoldier reviewed Dec 21, 2021

View reviewed changes

NullSoldier force-pushed the merkle_tree_contains branch from 4fc7839 to e5c131c Compare December 21, 2021 07:34

deekerno force-pushed the staging branch from 97ed51a to 34e688a Compare December 21, 2021 19:55

NullSoldier force-pushed the merkle_tree_contains branch 2 times, most recently from cfb9b4a to d10c055 Compare December 22, 2021 00:16

deekerno force-pushed the staging branch from 66501d3 to 37021f7 Compare December 22, 2021 04:01

dguenther force-pushed the staging branch from 61300d2 to dbbfdd1 Compare December 22, 2021 05:53

leanthebean and others added 4 commits December 21, 2021 22:51

removing pastSize from the contained method

630c5ab

Use existing merkle hash

17e5896

Fix dead lock

a557b5a

NullSoldier force-pushed the merkle_tree_contains branch from 6bd70f7 to a557b5a Compare December 22, 2021 06:52

NullSoldier added 2 commits December 21, 2021 22:58

Minor cleanup inside of verifier (#811)

1d7889e

Rename hasValidSpends to verifyConnectedSpends (#809)

e61da91

NullSoldier marked this pull request as draft December 22, 2021 07:21

dguenther reviewed Dec 22, 2021

View reviewed changes

dguenther approved these changes Dec 22, 2021

View reviewed changes

NullSoldier added 2 commits December 22, 2021 13:13

Only verify spend root in hasValidSpends (#810)

d110058

Truncate index during MerkleTree truncate (#820)

2d8c8ad

NullSoldier force-pushed the merkle_tree_contains branch from d68148f to 2d8c8ad Compare December 22, 2021 21:13

NullSoldier approved these changes Dec 22, 2021

View reviewed changes

NullSoldier marked this pull request as ready for review December 22, 2021 21:28

NullSoldier merged commit 427bfe8 into staging Dec 22, 2021

NullSoldier deleted the merkle_tree_contains branch December 22, 2021 21:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize MerkleTree.contains from O(n) to O(1) #775

Optimize MerkleTree.contains from O(n) to O(1) #775

leanthebean commented Dec 17, 2021

dguenther left a comment

dguenther Dec 18, 2021

NullSoldier Dec 21, 2021

dguenther commented Dec 18, 2021

NullSoldier commented Dec 21, 2021

NullSoldier Dec 21, 2021 •

edited

NullSoldier commented Dec 22, 2021

dguenther Dec 22, 2021

NullSoldier Dec 22, 2021

		@@ -296,16 +294,15 @@ export class Verifier {
		async hasValidSpends(block: Block, tx?: IDatabaseTransaction): Promise<VerificationResult> {

Optimize MerkleTree.contains from O(n) to O(1) #775

Optimize MerkleTree.contains from O(n) to O(1) #775

Conversation

leanthebean commented Dec 17, 2021

Summary

Testing Plan

Breaking Change

dguenther left a comment

Choose a reason for hiding this comment

dguenther Dec 18, 2021

Choose a reason for hiding this comment

NullSoldier Dec 21, 2021

Choose a reason for hiding this comment

dguenther commented Dec 18, 2021

NullSoldier commented Dec 21, 2021

NullSoldier Dec 21, 2021 • edited

Choose a reason for hiding this comment

NullSoldier commented Dec 22, 2021

dguenther Dec 22, 2021

Choose a reason for hiding this comment

NullSoldier Dec 22, 2021

Choose a reason for hiding this comment

NullSoldier Dec 21, 2021 •

edited