Check transaction signatures in entry verify

[sakridge]

Problem

Transaction signatures not verified.

Summary of Changes

Verify them in entry verification.

Fixes #8593

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (master@4f05f08). Click here to learn what that means.
The diff coverage is 97.5%.

@@           Coverage Diff            @@
##             master   #8596   +/-   ##
========================================
  Coverage          ?     80%           
========================================
  Files             ?     256           
  Lines             ?   55768           
  Branches          ?       0           
========================================
  Hits              ?   44651           
  Misses            ?   11117           
  Partials          ?       0

[garious]

That's a PoH Verify function. I would have expected it added in the TVU somewhere OR see SigVerifyStage moved to some shared location. Also, surprised to see CPU-based signature verification here. But given the central location, does this indicate that ledger-tool was skipping the verification as well?

I think it'd be best to address the architectural diagrams first on this one:

https://github.com/solana-labs/solana/blob/master/docs/art/validator.bob
https://github.com/solana-labs/solana/blob/master/docs/art/tpu.bob
https://github.com/solana-labs/solana/blob/master/docs/art/tvu.bob

[mvines]

does this indicate that ledger-tool was skipping the verification as well?

Yes, ledger-tool verify was not verifying the transaction signatures themselves. Arguably because by transactions should be all good by the time they hit blockstore, but yeah

[sakridge]

That's a PoH Verify function.

It was, now it's poh verify & transaction signature verify. There's nothing that says poh_verify that I noticed.

I would have expected it added in the TVU somewhere OR see SigVerifyStage moved to some
shared location. Also, surprised to see CPU-based signature verification here.

We can do it in a follow-up change, but this is the easiest/most straightforward way to add it. I did some perf sanity checks, we are still above 50k tps cpu-only on colo.

But given the central location, does this indicate that ledger-tool was skipping the verification as well?

Yes. This also solves this problem.

[garious]

This is an architecture bug more than it is an implementation bug. I don't think we should merge this until a change to architecture documents is approved. Zooming way, way in, all the way to function level (which is already too far), this patch makes the architectural decision to do signature verification before PoH verification. That alone looks like an attack vector (malicious leader randomly resembles transactions and floods the validators). At the very least, the code in this patch should be in a separate function, so that it's clear one form of verification is happening before the other.

[sakridge]

That alone looks like an attack vector (malicious leader randomly resembles transactions and floods the validators)

Why couldn't/wouldn't the malicious leader assemble those transaction to just pass poh verify? It just needs to do some hashing.

[garious]

The intent of my comment wasn't to debate an attack vector. It was to give a concrete example of how this innocent looking PR may have significant implications, and how a PR to an architectural document would be a better venue for that discussion.

Pull request has been modified.

[garious]

Thanks. While I would have rather discussed the change at the architectural level, this patch and tests are now so clear that anyone should easily be able to move it elsewhere, if needed.