-
Notifications
You must be signed in to change notification settings - Fork 296
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
Conversation
There was a problem hiding this 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.
aae204b
to
aa32398
Compare
@davecgh Thanks for the review. Makes sense regarding eventually separating the primary transaction data structure from the |
aa32398
to
41b298a
Compare
There was a problem hiding this 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.
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. |
There was a problem hiding this 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!
41b298a
to
220e9c8
Compare
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.
220e9c8
to
c8e946d
Compare
Also rebased to the latest |
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.
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).
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 block2
levels deep after processing a new block), with the following results:0
cache misses on signature hashes that were already evicted.1153
.Closes #2307