proof: add new IgnoreChecker proof rejection cache #1400

Roasbeef · 2025-02-19T02:39:14Z

In this PR, we introduce a new proof.VerifierCtx struct to cut down
on line noise when verifying single proofs and proof files. This also
lets us centralize the creation of the various interfaces we need to
verify a proof.

We then add a new interface, the IgnoreChecker. This is to be
used as a proof rejection cache, which allows us to ensure that we don't
continue to validate proofs that we already know to be invalid. The
underlying cache implementation is also intended to support its own
invalidation to handle re-org edge cases.

coveralls · 2025-02-19T02:55:37Z

Pull Request Test Coverage Report for Build 13563480812

Details

202 of 221 (91.4%) changed or added relevant lines in 17 files are covered.
No unchanged relevant lines lost coverage.
Overall first build on ignore-proof-checker at 54.588%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
proof/mock.go	11	12	91.67%
proof/verifier.go	46	48	95.83%
tapchannel/aux_funding_controller.go	0	8	0.0%
tapchannel/aux_sweeper.go	0	8	0.0%

Totals
Change from base Build 13541759190:	54.6%
Covered Lines:	48858
Relevant Lines:	89503

💛 - Coveralls

guggero

Thanks for the refactor, that was definitely needed!
I think this changes the caching behavior quite a bit, so we might want to revert small parts of it (see inline comment).

proof/append_test.go

proof/proof_test.go

guggero · 2025-02-19T08:17:09Z

proof/verifier.go

-		ctx, headerVerifier, merkleVerifier, groupVerifier,
-		chainLookupGen.GenFileChainLookup(&proofFile),
-	)
+	return proofFile.Verify(ctx, vCtx)


This changes the semantics on the use of ProofChainLookup. Before, we passed in the whole file, which actually made it useful as a cache, since it could look up TxBlockHeight in a previous proof of the file. Now that we always just pass in a single proof (within verifyAssetStateTransition), this caching effect isn't there anymore and we always need to hit the DB...

Perhaps we can keep the old signature on Proof.Verify() and just change the signature of the Verifier.Verify?

Ah, makes sense. I had an initial second thought once I needed to make those additional calls deeper in the proof verification sections.

I went back and forth a bit w.r.t if it should accept the ChainLookupGen, or just the ChainLookup interface.

Perhaps we can keep the old signature on Proof.Verify() and just change the signature of the Verifier.Verify?

I still think it's useful to be able to aggregate the set of dependent interfaces in a single place. One change I want to size up is if we can just have a global VerifierCtx in one place, so we don't need to re-create it in so many areas (ontriduces the chance that we forgot to populate some important, but optional interface).

Can you help me to understand this caching a bit better? Looking at ProofChainLookup, I see that we'll hit the assetStore to fetch the height of a given txid. I don't see how this portion was cached before. IIUC, my changes here amount to creating new sub-instance of ProofChainLookup when needed (no db calls in the constructor), vs passing around a pre-created instance.

So the chain lookup is used for three different things during (relative) lock time validation:

Get the current block height (easy, just a "cheap" call to the backend, no caching required)

Block mean time ("expensive" and not really cacheable, multiple calls to the backend required, but only required for unix timestamp based lock times)

Get block height of input transaction (requires txindex or chain re-scan, so considered too expensive), see

taproot-assets/vm/vm.go

Line 586 in e2107c8

inputConfirmHeight, err := chainLookup.TxBlockHeight(

The third one is the problem, as with Neutrino for example we can't do it in a reasonable time. BUT, that's where the proof file comes in: The previous proof in the file contains exactly the transaction we need plus its mined height. That's why we cache the full file whenever possible.

Thanks, for that explanation.

Pushed up a new version. I landed somewhere in the middle: VerifierCtx is still in place, but for proof.Verify it accepts the ChainLookup as another arg.

I considered making a new fn.Either[ChainLookup, ChainLookupGen] to make it clearer that they're used un a mutually exclusive way. Wasn't sure how people would feel about the ergonomics of that kind of approach though. I think we can mask some of the boiler plate with some helper methods. I lean towards this approach myself as it's less ambiguous which you should use (the generator or the ready instance).

guggero · 2025-02-19T08:20:18Z

proof/verifier.go

-			Index: p.InclusionProof.OutputIndex,
-		},
+		Asset:             &p.Asset,
+		OutPoint:          anchorOutPoint,


This commit doesn't compile, this variable doesn't exist yet.

proof/mock.go

proof/verifier.go

GeorgeTsagk

interesting aider approach, left some comments on the topic

.aider.conf.yml

.aider.chat.history.md

GeorgeTsagk · 2025-02-20T11:11:54Z

CONVENTIONS.md

+  • Include a prefix indicating the affected package (e.g. “lnwallet:” or “multi:” for changes spanning several packages).
+
+- **Ideal Git Commit Structure**  
+  • Make small, self-contained commits so that every commit builds on the previous one.  


i think we need to expand and emphasize a bit more on git commit structure

need to revolve more around small scope commits that build / make sense etc

didn't do a great job in this PR, everything is kind of mixed up

Removed as wasn't meant to be committed.

I had to do a big first commit, as it changed an interface that was widely used. If everything wasn't updated, then the commit wouldn't compile.

msg.txt

In this commit, we introduce a new proof.VerifierCtx struct to cut down on line noise when verifying single proofs and proof files. This also lets us centralize the creation of the various interfaces we need to verify a proof. Finally, this is a prep for an upcoming change where we'll add a new interface to the validation context.

In this commit, we add a new interface, the IgnoreChecker. This is to be used as a proof rejection cache, which allows us to ensure that we don't continue to validate proofs that we already know to be invalid. The underlying cache implementation is also intended to support its own invalidation to handle re-org edge cases. A concrete implementation will be added in an upcoming PR.

guggero

Pushed up a couple of fixes. With that, LGTM 🎉

guggero · 2025-02-27T09:05:45Z

proof/proof_stitching_test.go

-			context.Background(), verifier.VerifyHeader,
-			DefaultMerkleVerifier, MockGroupVerifier,
-			MockChainLookup,
+			context.Background(), vCtx,


Here we actually switch to the DefaultMerkleVerifier instead of the MockMerkleVerifier, since we now have a valid merkle proof (which IIRC is part of the test case).

guggero · 2025-02-27T09:13:44Z

proof/verifier.go

@@ -304,6 +308,12 @@ func (p *Proof) verifyAssetStateTransition(ctx context.Context,
 		splitAssets = append(splitAssets, splitAsset)
 	}

+	chainLookup, err := vCtx.ChainLookupGen.GenProofChainLookup(p)


We should actually pass in the chainLookup from Verify. Otherwise the caching still doesn't work.

GeorgeTsagk

Lgtm
looking forward to the implementation

Roasbeef added enhancement New feature or request proofs unified universe commitment labels Feb 19, 2025

Roasbeef requested review from a team, guggero and GeorgeTsagk and removed request for a team February 19, 2025 02:39

guggero requested changes Feb 19, 2025

View reviewed changes

Roasbeef force-pushed the ignore-proof-checker branch from 7c3f579 to 3d8b464 Compare February 20, 2025 01:57

GeorgeTsagk reviewed Feb 20, 2025

View reviewed changes

msg.txt Outdated Show resolved Hide resolved

levmi assigned Roasbeef Feb 20, 2025

Roasbeef force-pushed the ignore-proof-checker branch 3 times, most recently from fb3f5c2 to 5b68d1d Compare February 22, 2025 03:26

Roasbeef requested review from guggero and GeorgeTsagk February 24, 2025 20:27

Roasbeef added 2 commits February 27, 2025 10:44

guggero force-pushed the ignore-proof-checker branch from 5b68d1d to 1cfbe7c Compare February 27, 2025 09:47

guggero approved these changes Feb 27, 2025

View reviewed changes

GeorgeTsagk approved these changes Feb 27, 2025

View reviewed changes

guggero merged commit 8b2965f into lightninglabs:main Feb 27, 2025
18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

proof: add new IgnoreChecker proof rejection cache #1400

proof: add new IgnoreChecker proof rejection cache #1400

Roasbeef commented Feb 19, 2025

coveralls commented Feb 19, 2025 •

edited

Loading

guggero left a comment

guggero Feb 19, 2025

Roasbeef Feb 20, 2025

guggero Feb 20, 2025

Roasbeef Feb 24, 2025

guggero Feb 19, 2025

GeorgeTsagk left a comment

GeorgeTsagk Feb 20, 2025

Roasbeef Feb 21, 2025

guggero left a comment

guggero Feb 27, 2025

guggero Feb 27, 2025

GeorgeTsagk left a comment

proof: add new IgnoreChecker proof rejection cache #1400

proof: add new IgnoreChecker proof rejection cache #1400

Conversation

Roasbeef commented Feb 19, 2025

coveralls commented Feb 19, 2025 • edited Loading

Pull Request Test Coverage Report for Build 13563480812

Details

💛 - Coveralls

guggero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GeorgeTsagk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guggero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GeorgeTsagk left a comment

Choose a reason for hiding this comment

coveralls commented Feb 19, 2025 •

edited

Loading