-
Notifications
You must be signed in to change notification settings - Fork 133
proof: implement and hook up ignore checker #1716
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
base: main
Are you sure you want to change the base?
Conversation
Pull Request Test Coverage Report for Build 16940968541Details
💛 - Coveralls |
To better support the actual implementation of the ignore checker, we add a context and also allow returning an error through the use of a fn.Result return type.
To allow a proof to be validated at least partially, we extract all the steps (1 through 7) into its own method, called VerifyProofIntegrity. These steps are all verification steps that can be executed without needing access to the previous asset snapshot (the inputs being spent), which aren't available in some situations. This refactor allows us to make sure a proof is structurally sound (e.g. all Taproot/Tapscript inclusion/exclusion proofs are valid) and the proof isn't being ignored.
To make sure ignored assets can't be sent out in the first place, we add the better proof integrity check that includes the ignore check to the asset wallet.
To make sure we don't always hit the DB when we encounter a new asset point that is _not_ ignored, we add an LRU cache that tracks asset points that are currently _not_ ignored. The cache will be invalidated by the supply verifier that watches the chain for new supply commitment updates and is aware of new potential ignore tuples.
This commit makes the negative lookup LRU cache size configurable by the user.
With this commit we hook up the different caches we have in the multiverse store and the supply ignore checker to the Prometheus metrics collector.
To make sure we don't have old entries in our negative lookup cache, we invalidate the ignore checker's LRU cache as soon as we have written a new supply update to disk.
717b0ef
to
2da4253
Compare
Added the negative LRU cache, hooked up the cache stats to our Prometheus metrics and implemented the cache invalidation. |
// FetchSupplyLeavesByType fetches all supply leaves for a given asset specifier | ||
// and a specific supply sub-tree. | ||
func (s *SupplyTreeStore) FetchSupplyLeavesByType(ctx context.Context, | ||
spec asset.Specifier, tree supplycommit.SupplySubTree, startHeight, |
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.
tree -> treeType ?
@@ -45,7 +45,7 @@ type IgnoreChecker interface { | |||
// IsIgnored returns true if the given prevID is known to be invalid. A | |||
// prevID is used here, but the check should be tested against a proof | |||
// result, or produced output. | |||
IsIgnored(prevID AssetPoint) bool | |||
IsIgnored(ctx context.Context, prevID AssetPoint) lfn.Result[bool] |
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.
👍
// String returns a human-readable description of the PrevID. | ||
func (id PrevID) String() string { | ||
return fmt.Sprintf("PrevID(outpoint=%s, id=%s, script_key=%x)", | ||
id.OutPoint.String(), id.ID.String(), id.ScriptKey.CopyBytes()) |
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.
CopyBytes()
as an alternative to just slicing into the underlying byte slice?
// IgnoreCheckerCfg is a configuration struct for the CachingIgnoreChecker. | ||
type IgnoreCheckerCfg struct { | ||
// GroupQuery is used to query asset groups by their asset ID. | ||
GroupQuery assetGroupQuery |
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.
Hmm, if the config struct itself is publicly exported, then shouldn't the interfaces exported also be?
// group (or 0 if we haven't encountered this group yet). | ||
specifier := asset.NewSpecifierFromGroupKey(*groupPubKey) | ||
bestHeight := c.bestHeightLookup[groupKey] | ||
leaves, err := c.cfg.Store.FetchSupplyLeavesByType( |
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.
One alternative here would be doing a more precise query just that that particular ignore point. On the other ahdn wit hthis approach, we do the broad look up once, then just cache it from there on.
Store: supplyTreeStore, | ||
}) | ||
|
||
var ignoreCheckerIface proof.IgnoreChecker = ignoreChecker |
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.
An alternative to adding an explicit type param to the lfn.Some
call below?
return nil, fmt.Errorf("input proof is nil") | ||
} | ||
|
||
_, err := assetProof.VerifyProofIntegrity(ctx, vCtx) |
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.
Nice to do additional verification here as well.
withError("is ignored"), | ||
) | ||
|
||
// TODO(ffranr): The above only tests that the node that issued the |
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.
Can make an issue for this 👍
@@ -3,8 +3,10 @@ package tapdb | |||
import ( | |||
"context" | |||
"errors" | |||
"fmt" |
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.
Re the commit message: do we need a supply verifier to invalidate it? Couldn't we just invalidate it selectively when/if an asset in that group is ignored?
// We know our tree has been updated, so we need to make sure | ||
// the negative lookup cache of the ignore checker is flushed | ||
// and the new ignore leaves are loaded from disk. | ||
env.IgnoreCheckerCache.InvalidateCache() |
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.
Couldn't we just selectively invalidate the cache for this particular group key?
Even before that, we can check to see if this batch adds any new ignored assets, only invalidating if so.
Fixes #1684.
Implements and hooks up a caching ignore tree checker.