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

txscript: Proactively evict SigCache entries. #2358

Merged
merged 3 commits into from
Sep 24, 2020

Conversation

rstaudt2
Copy link
Member

@rstaudt2 rstaudt2 commented Sep 18, 2020

This adds functionality to proactively evict SigCache entries when they are nearly guaranteed to no longer be useful. It accomplishes this by evicting entries related to transactions in the block that is 2 levels deep from a newly processed block.

Proactively evicting entries reduces the likelihood of the SigCache reaching maximum capacity quickly and then relying on random eviction, which may randomly evict entries that are still useful.


Implementation

There are a few different approaches that were considered for implementing the logic to determine which SigCache entries to proactively remove based on the transactions in a given block, with various tradeoffs:

  • Generate all signature hashes for all transactions within the block, and evict based on the signature hashes.

    • Pros
      • Does not require any additional space in the SigCache.
      • SigCache is already keyed on the signature hash so lookup is constant time.
    • Cons
      • Calculating the signature hash is pretty slow (though @davecgh has some proposed improvements in Improved Signature Hash Algorithm Proposal #950).
      • The signature hash code is necessarily coupled with the rest of the opcode parsing and execution logic, so the complexity would be higher to either re-run through all of this code or refactor the necessary bits out for re-use.
  • Generate a transaction hash for all transactions within the block, and evict based on the transaction hash that is additionally stored within the SigCache entries (implemented in this PR).

    • Pros
      • The logic is very easy to follow, as it only involves taking a hash of the transaction and evicting based on that hash.
      • Uses SipHash-2-4 to create a "short" tx hash, which is very fast, takes up less space, and the algorithm is already vetted and in-use (suggested as a potential option by @davecgh).
    • Cons
      • Takes extra space to store the short tx hash in the SigCache - ~6% overhead:
        • Each cache entry is ~137 bytes currently (32-byte sighash key + 33-byte public key + ~71-73-byte signature (on average)). Adding an 8 byte short hash is ~6% overhead (8/137 = .058)

Testing

I used this debug code to track signature hashes that have been evicted from the SigCache and log a warning if there is a cache miss on any of the signature hashes that were already evicted. I also used the debug code to track the most entries that were in the cache at any point in time.

In addition to testing various scenarios in simnet, I ran a node on mainnet for 5 days with ProactiveEvictionDepth = 2 (evict entries related to transactions in the block 2 levels deep after processing a new block), with the following results:

  • There were 0 cache misses on signature hashes that were already evicted.
  • The most entries in the cache at any point in time was 1153.

Closes #2307

Copy link
Member

@davecgh davecgh 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 great overall. It's easy to follow and incorporates the things we've previously discussed. I also appreciate you took the time to analyze the runtime behavior and prove the expectation of eviction semantics based on educated guessing do in fact match up with reality.

While I understand the desire to do the short hashing stuff in wire so it's defined on the MsgTx type and thus accessible via tx.ShortTxHash, I really think it probably belongs over in txscript which is the only place it will be used. That would also remove the need for all of the (temporary) module overrides.

I do realize that means it will have to be:

shortHash := ShortTxHash(tx, key)

versus the slightly more ergonomic

shortHash := tx.ShortTxHash(key)

However, we don't want to give the impression that this short tx hash is something that should be used in the wire protocol, and also, I'm pretty keen to avoid adding anything to wire that would end up requiring a major module version bump.

I think the larger issue, which is something I've had on my agenda for a while, is that MsgTx kind of doubles as the primary data structure for transactions as well as what is encoded to the wire and they really ought to be separate. That is a much bigger discussion and huge change though and thus not something that would go in this PR by any means.

blockchain/common_test.go Outdated Show resolved Hide resolved
blockchain/fullblocks_test.go Outdated Show resolved Hide resolved
txscript/sigcache_test.go Outdated Show resolved Hide resolved
txscript/sigcache_test.go Outdated Show resolved Hide resolved
txscript/sigcache_test.go Outdated Show resolved Hide resolved
blockmanager.go Show resolved Hide resolved
blockmanager.go Outdated Show resolved Hide resolved
txscript/sigcache_test.go Outdated Show resolved Hide resolved
@rstaudt2
Copy link
Member Author

@davecgh Thanks for the review. Makes sense regarding eventually separating the primary transaction data structure from the wire protocol. I had put the shortTxHash functionality in MsgTx since I thought it could be useful in other places in the future, but I agree that we don't want it to be confused for something that should be used in the wire protocol. I moved it to txscript since that's the only place it is being used.

txscript/sigcache.go Show resolved Hide resolved
Copy link
Member

@davecgh davecgh left a comment

Choose a reason for hiding this comment

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

Great work. Reviewed the latest and did some testing on it.

@davecgh davecgh added this to the 1.6.0 milestone Sep 20, 2020
@davecgh
Copy link
Member

davecgh commented Sep 20, 2020

I went ahead and marked this to go in for 1.6.0 since we're still waiting on the Treasury stuff to finalize and this is good to go.

Copy link
Member

@matheusd matheusd left a comment

Choose a reason for hiding this comment

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

Don't feel that you need the things I'm pointing out, as they are only stylistic in nature.

Nice work overall!

txscript/sigcache.go Outdated Show resolved Hide resolved
txscript/sigcache.go Outdated Show resolved Hide resolved
This defines a shortTxHash method that generates a short 64-bit hash
from the standard 128-bit hash.  It accomplishes this by using the
SipHash-2-4 hash function.

This is being introduced to track the transaction associated with a
signature cache entry at a reduced memory overhead.
This adds a uint64 short transaction hash to sigcache entries.  This can
be used to proactively evict entries from the sigcache based on the
transaction that they are associated with.
This adds functionality to proactively evict SigCache entries when they
are nearly guaranteed to no longer be useful.  It accomplishes this by
evicting entries related to transactions in the block that is 2 levels
deep from a newly processed block.

Proactively evicting entries reduces the likelihood of the SigCache
reaching maximum capacity quickly and then relying on random eviction,
which may randomly evict entries that are still useful.
@rstaudt2
Copy link
Member Author

Also rebased to the latest master and blockchain/standalone/go.sum needed to be updated as part of that rebase.

@davecgh davecgh merged commit df141bb into decred:master Sep 24, 2020
@rstaudt2 rstaudt2 deleted the proactive-sigcache-eviction branch January 15, 2021 12:43
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.

Proactive signature cache eviction.
5 participants