Add MMD verification in CT Hammer #167

roger2hk · 2025-03-07T15:40:01Z

mhutchinson

I'm happy with the shape of this, so nicely done. There are a few issues around parallelism that give me some concern though, so PTAL at my comments. We can discuss 1:1 if you want more details.

internal/hammer/loadtest/client.go

internal/hammer/loadtest/hammer.go

internal/hammer/loadtest/workers.go

phbnf · 2025-03-10T12:12:18Z

internal/hammer/loadtest/workers.go

+}
+
+// entryFromChain generates an Entry from a chain and timestamp.
+// copied from certificate-transparency-go/serialization.go


Just to keep track of things, can you add a comment / TODO to reflect that it comes form handlers.go?

Added the comment.

phbnf · 2025-03-10T12:28:53Z

internal/hammer/loadtest/workers.go

+				// Verify MMD timestamp.
+				sec := int64(leafMMD.timestamp / 1000)
+				nsec := int64((leafMMD.timestamp % 1000) * 1000000)
+				if time.Unix(sec, nsec).Add(v.mmdDuration).Before(time.Now()) {


I think you can use time.UnixMilli directly with leafMMD.timestamp.

Good call, updated.

phbnf · 2025-03-11T10:33:42Z

internal/hammer/loadtest/workers.go

@@ -258,8 +258,7 @@ func (w *LogWriter) Run(ctx context.Context) {
 		select {
 		case w.leafMMDChan <- LeafMMD{chain, index, timestamp}:
 		default:
-			// Drop if leafMMDChan is full. This could happen if the number
-			// of MMD verifiers is smaller than the number of writers.
+			// Drop if leafMMDChan is full. This could happen if the MMD verifiers are falling behind.


Can you log something to make sure this doesn't happen too silently? Either here, or at read time if this is more convenient.

Added the log when the leafMMDChan is full.

phbnf · 2025-03-11T11:29:50Z

internal/hammer/loadtest/hammer.go

 }

 func NewHammer(tracker *client.LogStateTracker, f client.EntryBundleFetcherFunc, w LeafWriter, gen func() []byte, seqLeafChan chan<- LeafTime, errChan chan<- error, opts HammerOpts) *Hammer {
 	readThrottle := NewThrottle(opts.MaxReadOpsPerSecond)
 	writeThrottle := NewThrottle(opts.MaxWriteOpsPerSecond)

+	leafMMDChan := make(chan LeafMMD, opts.NumWriters)


Should this be higher? It looks to me that this channel will get full, new leafMMD won't be added to it, and this behavior would go silent. As we spoke about, logging a message each time a leafMMD would spam logs when MMDVerifiers are not enabled. I think that would be fine provided that the hammer should always have them on. Alternatively, you could compare the length and capacity of the channel at read time to tell whether or not it's full, and indicate to the operator that they might want to increase the channel size?

If there aren't enough MMDVerifier workers (which could be the case because integration is slower than sequencing) and the channel gets full, you might end up always verifying MMDs after MMD+X time has passed, and therefore never detect if there is an MMD violation smaller between [0;X]

Bumped the leafMMDChan buffer size to a huge size. I've stop logging when the number of MMD verifiers is zero.

phbnf · 2025-03-11T11:49:09Z

internal/hammer/loadtest/hammer.go

 	randomReaders := NewWorkerPool(func() Worker {
 		return NewLeafReader(tracker, f, RandomNextLeaf(), readThrottle.TokenChan, errChan)
 	})
 	fullReaders := NewWorkerPool(func() Worker {
 		return NewLeafReader(tracker, f, MonotonicallyIncreasingNextLeaf(), readThrottle.TokenChan, errChan)
 	})
 	writers := NewWorkerPool(func() Worker {
-		return NewLogWriter(w, gen, writeThrottle.TokenChan, errChan, seqLeafChan)
+		return NewLogWriter(w, gen, writeThrottle.TokenChan, errChan, seqLeafChan, leafMMDChan)


In a followup PR, you could also add a leafMMDChan to randomReaders. MMDVerifier does two things really:

checks that input matches the hases in the tree

make sure that 1. passes within the MMD

From writer, input is what the hammer has generated - and that's great! After that check, it would still be possible for leaf bundles and L0 hashes not to match.
From reader, input would be leaf bundles read from the tree. Leaf bundles and L0 hashes must match for this test to pass.

phbnf · 2025-03-11T11:59:02Z

internal/hammer/loadtest/client.go

 	}
-	index, err := parseAddChainResponse(body)
+	index, timestamp, err := parseAddChainResponse(body)


Maybe in a followup PR, you could check here that the SCT matches with the request.

internal/hammer/loadtest/workers.go

internal/hammer/loadtest/hammer.go

… worker unexpectedly

internal/hammer/loadtest/workers.go

roger2hk force-pushed the correctness-test branch 6 times, most recently from 1155b09 to f774a3a Compare March 7, 2025 23:54

Add MMD verification in CT Hammer

6dc37e7

roger2hk force-pushed the correctness-test branch from f774a3a to 6dc37e7 Compare March 7, 2025 23:55

roger2hk requested review from phbnf and mhutchinson March 7, 2025 23:56

roger2hk assigned mhutchinson and phbnf Mar 7, 2025

roger2hk marked this pull request as ready for review March 7, 2025 23:56

mhutchinson reviewed Mar 10, 2025

View reviewed changes

phbnf reviewed Mar 10, 2025

View reviewed changes

roger2hk added 7 commits March 10, 2025 16:04

Rearrange the NewMMDVerifier arguments

a5a6681

Update comment wording when leafMMDChan is full

c0b1c95

Break if there is any error during MMD verification

d1c6363

Replace time.Unix with time.UnixMilli

e13d3fe

Update comment for the use of ProofBuilder in MMD verifier

8736695

Specify leafMMDChan channel direction in NewMMDVerifier

5e70a73

Update comment for the use of ProofBuilder in MMD verifier

c8939d3

roger2hk requested review from mhutchinson and phbnf March 10, 2025 20:41

phbnf reviewed Mar 11, 2025

View reviewed changes

Simplify the MMDVerifier loop

22624bb

phbnf reviewed Mar 11, 2025

View reviewed changes

roger2hk added 3 commits March 11, 2025 12:00

Bump the buffer size of leafMMDChan

c6c42a1

Log when leafMMDChan is full

643cbbf

Stop logging when the number of MMD verifiers is 0

fd99c44

Add a TODO to exit gracefully when the inclusion proof fails

8c46bdd

mhutchinson reviewed Mar 11, 2025

View reviewed changes

internal/hammer/loadtest/workers.go Outdated Show resolved Hide resolved

Move on to the next leaf if inclusion proof fails

eaa9975

mhutchinson reviewed Mar 11, 2025

View reviewed changes

internal/hammer/loadtest/hammer.go Outdated Show resolved Hide resolved

roger2hk added 6 commits March 11, 2025 12:31

Use the cap instead of len for leafMMDChan check

7fb3c47

Revert the leafMMDChan buffer size back to number of writers

091816d

Fix nil pointer when there is nothing in the leafMMDChan

3c18e92

Replace break with continue in MMDVerifier as break stops the…

304dc0a

… worker unexpectedly

Add more comments for MMD verification

02f7de7

Bump the leafMMDChan buffer size to NumWriters*12

8fb6474

roger2hk requested a review from mhutchinson March 11, 2025 15:43

Lower the leafMMDChan buffer size to NumWriters*2

4b8fb44

mhutchinson reviewed Mar 11, 2025

View reviewed changes

internal/hammer/loadtest/workers.go Outdated Show resolved Hide resolved

roger2hk added 2 commits March 11, 2025 16:27

Remove default case in MMDVerifier

593fad1

Reuse and update checkpoint and proof builder only if needed

b1154cf

mhutchinson approved these changes Mar 11, 2025

View reviewed changes

roger2hk requested a review from phbnf March 11, 2025 17:47

phbnf approved these changes Mar 11, 2025

View reviewed changes

roger2hk merged commit 560d6f4 into transparency-dev:main Mar 11, 2025
7 checks passed

roger2hk deleted the correctness-test branch March 11, 2025 18:14

Add MMD verification in CT Hammer #167

Add MMD verification in CT Hammer #167

Uh oh!

Conversation

roger2hk commented Mar 7, 2025

Uh oh!

mhutchinson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!