FVM tx verifier improvements #8335
Conversation
📝 WalkthroughWalkthroughAdds two validator error types, enforces that transaction signatures must belong to proposer/payer/authorizers, validates signature formats, refactors FVM signature processing to deduplicate and aggregate weights before verification, and updates tests and fixtures to cover missing/unrelated-signature scenarios. Changes
Sequence DiagramsequenceDiagram
participant Submitter as TransactionSubmitter
participant Validator as AccessValidator
participant FVM as FVMVerifier
participant KeyStore as AccountKeyLookup
participant Crypto as CryptoVerifier
Submitter->>Validator: submit(tx)
Validator->>Validator: checkAccounts(tx)
alt unrelated signature detected
Validator-->>Submitter: UnrelatedAccountSignatureError
else missing required signature
Validator-->>Submitter: MissingSignatureError
else formats OK
Validator->>Validator: checkSignatureFormat(tx)
Validator-->>FVM: forward tx
FVM->>FVM: newSignatureEntries(tx) (dedupe)
FVM->>KeyStore: getAccountKeysAndAggregateWeights(signatures)
KeyStore-->>FVM: keys + aggregated weights
FVM->>FVM: enforce weight thresholds
FVM->>Crypto: verifySignatures(concurrent)
Crypto-->>FVM: verification results
FVM-->>Submitter: final verification result / error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
tarakby
requested review from
janezpodhostnik,
m-Peter,
peterargue and
zhangchiqing
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
fvm/transactionVerifier.go (1)
62-87: Missing early return after extension data validation failure.When
ValidateExtensionDataAndReconstructMessagereturnsvalid=false(lines 70-72), the code setsverifyErrbut continues toVerifySignatureFromTransactionwith a potentially undefinedmessage. The verification should return early after detecting invalid extension data.Proposed fix
entry.invokedVerify = true valid, message := entry.ValidateExtensionDataAndReconstructMessage(entry.payload) if !valid { entry.verifyErr = entry.newError(fmt.Errorf("signature extension data is not valid")) + return entry.verifyErr } valid, err := crypto.VerifySignatureFromTransaction(
🤖 Fix all issues with AI agents
In `@access/validator/validator.go`:
- Line 446: Fix the typo in the comment above the checkSignatureFormat function:
change "idependently" to "independently" in the comment that reads "//
checkSignatureFormat checks the format of each transaction signature
idependently."
In `@fvm/transactionVerifier_test.go`:
- Around line 393-419: Fix the typo in the test assertion message inside the
weights/signatures test: change "invald signature" to "invalid signature" in the
assert.ErrorContains call to make the message accurate. Also declare the error
variable locally when calling run by using := for err (e.g., err := run(tx, ctx,
txnState)) so the test doesn't rely on an outer-scoped err variable.
- Around line 421-478: Fix the small typo in the comment for the authorizer
signature (change "authotizer" to "authorizer") and avoid reusing the
outer-scope err variable by declaring a new local error for each run call inside
the test: replace both instances of "err = run(tx, ctx, txnState)" with local
declarations "err := run(tx, ctx, txnState)" so the two checks after the payload
and envelope signature setups use their own err variables; the changes apply
inside the t.Run("signature from unrelated address", ...) block around the two
run(...) calls and the comment for sig3.
🧹 Nitpick comments (2)
access/validator/validator.go (1)
236-236: Metric label may not reflect new error type.The
checkAccountsfunction now returns bothDuplicatedSignatureErrorandUnrelatedAccountSignatureError, but the fail reason metric is onlymetrics.DuplicatedSignature. Consider whether a new metric label for unrelated signatures would provide better observability.fvm/transactionVerifier_test.go (1)
246-300: Good coverage for missing authorizer signatures.This test properly verifies that transactions fail when an authorizer (
address4) exists in the authorizer list but has no corresponding signature.Minor: Consider using
privKey2.HashAlgo(like line 133) instead of hardcodedhash.SHA3_256for consistency with other tests, though both should work given default fixtures.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
access/validator/errors.goaccess/validator/validator.goengine/collection/ingest/engine_test.gofvm/transactionVerifier.gofvm/transactionVerifier_test.gointegration/dkg/dkg_emulator_suite.gointegration/tests/collection/suite.go
🧰 Additional context used
📓 Path-based instructions (8)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/coding_conventions.mdc)
Follow Go coding conventions as documented in
@docs/agents/CodingConventions.mdFollow Go coding standards and conventions as documented in
@docs/agents/GoDocs.md
**/*.go: Follow the existing module structure in/module/,/engine/,/model/and use dependency injection patterns for component composition
Implement proper interfaces before concrete types
Follow Go naming conventions and the project's coding style defined in /docs/CodingConventions.md
Use mock generators: runmake generate-mocksafter interface changes
All inputs must be considered potentially byzantine; error classification is context-dependent and no code path is safe unless explicitly proven and documented
Use comprehensive error wrapping for debugging; avoidfmt.Errorf, useirrecoverablepackage for exceptions
NEVER log and continue on best effort basis; ALWAYS explicitly handle errors
Uses golangci-lint with custom configurations (.golangci.yml) and custom linters for Flow-specific conventions (struct write checking)
Files:
access/validator/errors.goengine/collection/ingest/engine_test.gointegration/dkg/dkg_emulator_suite.gofvm/transactionVerifier_test.gointegration/tests/collection/suite.goaccess/validator/validator.gofvm/transactionVerifier.go
{crypto,fvm,ledger,access,engine}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Cryptographic operations require careful handling; refer to crypto library documentation for proper implementation
Files:
access/validator/errors.goengine/collection/ingest/engine_test.gofvm/transactionVerifier_test.goaccess/validator/validator.gofvm/transactionVerifier.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Unit tests should be co-located with the code they test
Follow the existing pattern of*_test.gofiles for test naming
Use fixtures for realistic test data as defined in/utils/unittest/
Files:
engine/collection/ingest/engine_test.gofvm/transactionVerifier_test.go
{module,engine,cmd}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
All major processing components must implement the
Componentinterface from/module/component/component.goto ensure consistent lifecycle management and graceful shutdown patterns
Files:
engine/collection/ingest/engine_test.go
{network,engine,consensus}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Network messages must be authenticated and validated
Files:
engine/collection/ingest/engine_test.go
{crypto,fvm,ledger,storage}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Security checks for cryptographic misuse must be enforced using gosec
Files:
fvm/transactionVerifier_test.gofvm/transactionVerifier.go
{storage,ledger,execution,fvm}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
State consistency is paramount; use proper synchronization primitives
Files:
fvm/transactionVerifier_test.gofvm/transactionVerifier.go
integration/tests/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Integration tests should go in
/integration/tests/
Files:
integration/tests/collection/suite.go
🧠 Learnings (3)
📚 Learning: 2025-12-23T00:28:41.005Z
Learnt from: CR
Repo: onflow/flow-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-23T00:28:41.005Z
Learning: Applies to {crypto,fvm,ledger,access,engine}/**/*.go : Cryptographic operations require careful handling; refer to crypto library documentation for proper implementation
Applied to files:
fvm/transactionVerifier_test.go
📚 Learning: 2025-12-23T00:28:41.005Z
Learnt from: CR
Repo: onflow/flow-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-23T00:28:41.005Z
Learning: Applies to {crypto,fvm,ledger,storage}/**/*.go : Security checks for cryptographic misuse must be enforced using gosec
Applied to files:
fvm/transactionVerifier_test.go
📚 Learning: 2025-12-23T00:28:41.005Z
Learnt from: CR
Repo: onflow/flow-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-23T00:28:41.005Z
Learning: Applies to {network,engine,consensus}/**/*.go : Network messages must be authenticated and validated
Applied to files:
fvm/transactionVerifier_test.go
🧬 Code graph analysis (4)
engine/collection/ingest/engine_test.go (2)
utils/unittest/fixtures.go (3)
TransactionBodyFixture(1598-1614)TransactionSignatureFixture(121-136)RandomAddressFixture(77-79)access/validator/errors.go (1)
UnrelatedAccountSignatureError(82-84)
fvm/transactionVerifier_test.go (7)
fvm/context.go (6)
Context(32-54)NewContext(57-59)WithAuthorizationChecksEnabled(278-283)WithAccountKeyWeightThreshold(299-304)WithSequenceNumberCheckAndIncrementEnabled(287-292)WithTransactionBodyExecutionEnabled(310-315)utils/unittest/fixtures.go (1)
RandomAddressFixture(77-79)utils/unittest/fixtures/util.go (1)
NoError(6-10)utils/unittest/fixtures/signature.go (1)
Signature(8-8)fvm/crypto/hash.go (1)
NewPrefixedHashing(63-81)model/flow/constants.go (1)
TransactionTagString(79-79)utils/unittest/fixtures/crypto.go (1)
PrivateKey(5-5)
integration/tests/collection/suite.go (2)
fvm/script.go (1)
Script(26-34)engine/execution/testutil/fixtures.go (1)
SignEnvelope(127-140)
access/validator/validator.go (2)
module/metrics/labels.go (1)
DuplicatedSignature(177-177)access/validator/errors.go (1)
UnrelatedAccountSignatureError(82-84)
🔇 Additional comments (12)
access/validator/errors.go (1)
80-88: LGTM!The new
UnrelatedAccountSignatureErrortype follows the established pattern of other error types in this file and provides a clear, descriptive error message.integration/tests/collection/suite.go (1)
181-185: LGTM!Clearing
EnvelopeSignaturesbefore re-signing is correct. This prevents stale signatures from accumulating during the hash-grinding loop, which would now fail the new unrelated account signature validation.access/validator/validator.go (2)
415-444: LGTM!The
checkAccountsfunction correctly validates both duplicate signatures and unrelated account signatures. The related accounts set properly includes the payer, proposer, and all authorizers.
453-495: LGTM!The signature format validation correctly validates extension data for payload/envelope signatures separately, then checks if each cryptographic signature could be a valid ECDSA signature for either supported curve.
engine/collection/ingest/engine_test.go (1)
260-291: LGTM!The new test cases correctly validate that
UnrelatedAccountSignatureErroris returned when signatures from unrelated accounts are included in either envelope or payload signatures. The test setup properly configures related accounts before adding an unrelated signature.fvm/transactionVerifier.go (4)
89-176: LGTM!The refactored
newSignatureEntriesfunction correctly validates that all signatures come from related accounts (payer, proposer, or authorizers) and detects duplicate signatures. The weight maps are properly initialized for later population during key resolution.
273-278: Verify error wrapping is intentional.Wrapping
verifySignatureserrors withNewInvalidProposalSignatureErrormay be misleading when the failing signature is not the proposal signature. Consider if a more specific error type would be appropriate for non-proposal signature failures.
283-321: LGTM!The function correctly fetches account keys and aggregates weights into the shared weight maps. Weight aggregation during key resolution is efficient and avoids a separate pass.
323-407: LGTM!The concurrent signature verification with early cancellation is well-implemented. The deterministic error output is correctly maintained by waiting for all workers and returning the first error in signature order.
fvm/transactionVerifier_test.go (2)
21-39: Well-structured test helpers.The
newContext()andnewAccount()helpers effectively reduce boilerplate and improve test readability. ThefullWeightconstant provides a clear baseline for weight calculations.
302-358: Solid test coverage for weight threshold enforcement.These tests properly verify that:
- Authorizers with partial weights below the threshold are rejected
- Payers with partial weights are rejected
- Weight validation occurs before cryptographic signature verification
This aligns well with the PR objective of improving FVM transaction verification.
Also applies to: 360-391
integration/dkg/dkg_emulator_suite.go (1)
358-361: Correct adaptation to new signature validation.The removal of the DKG address and signer from
prepareAndSubmitis appropriate since the DKG address is not a party to this transaction (not payer, proposer, or authorizer). The transaction structure confirms:
- Payer:
node.account.accountAddress→ signs envelope- Proposer:
s.serviceAccountAddress→ signs payload- Authorizer:
node.account.accountAddress→ covered by envelope signatureThis aligns with the new validation that rejects signatures from unrelated accounts.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
fvm/transactionVerifier.go (1)
69-86: Missing early return after extension data validation failure.When extension data validation fails on line 71,
entry.verifyErris set but execution continues to the cryptographic verification on line 74. This wastes computation and may overwrite the original error if the crypto verification also fails.Suggested fix
valid, message := entry.ValidateExtensionDataAndReconstructMessage(entry.payload) if !valid { entry.verifyErr = entry.newError(fmt.Errorf("signature extension data is not valid")) + return entry.verifyErr } valid, err := crypto.VerifySignatureFromTransaction(
♻️ Duplicate comments (2)
fvm/transactionVerifier_test.go (2)
421-478: Variable scoping improvements needed.Lines 469 and 476 use the outer scope
errvariable. Consider using local declarations for cleaner test isolation:Suggested fix
ctx := newContext() - err = run(tx, ctx, txnState) + err := run(tx, ctx, txnState) assert.ErrorContains(t, err, "that is neither payer nor authorizer nor proposer", "error should be about unrelated account signature") // unrelated account signature is included as an envelope signature tx.PayloadSignatures = []flow.TransactionSignature{sig2, sig3} tx.EnvelopeSignatures = []flow.TransactionSignature{sig1, sig4} - err = run(tx, ctx, txnState) + err = run(tx, ctx, txnState) // reuse err from above since second check in same test assert.ErrorContains(t, err, "that is neither payer nor authorizer nor proposer", "error should be about unrelated account signature")
393-419: Typo and variable scoping issue.Line 418 has a typo: "invald signature" should be "invalid signature".
Additionally, line 417 uses the outer scope
errvariable instead of declaring locally with:=.Suggested fix
ctx := newContext() - err = run(tx, ctx, txnState) - assert.ErrorContains(t, err, "payer account does not have sufficient signatures", "error should be about insufficient payer weights not invald signature") + err := run(tx, ctx, txnState) + assert.ErrorContains(t, err, "payer account does not have sufficient signatures", "error should be about insufficient payer weights not invalid signature")
🧹 Nitpick comments (1)
fvm/transactionVerifier.go (1)
395-407: Consider returning an error instead of panic.The
panicon line 406 is intended as an unreachable safeguard. While the logic guarantees this path won't execute (iffoundErroris true, at least one entry hasverifyErr != nil), replacing it with an explicit error return would be more defensive:for _, entry := range signatures { if entry.verifyErr != nil { return entry.verifyErr } } - panic("Should never reach here") + return fmt.Errorf("internal error: expected verification error not found") }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
fvm/transactionVerifier.gofvm/transactionVerifier_test.gointegration/tests/collection/suite.go
🚧 Files skipped from review as they are similar to previous changes (1)
- integration/tests/collection/suite.go
🧰 Additional context used
📓 Path-based instructions (5)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/coding_conventions.mdc)
Follow Go coding conventions as documented in
@docs/agents/CodingConventions.mdFollow Go coding standards and conventions as documented in
@docs/agents/GoDocs.md
**/*.go: Follow the existing module structure in/module/,/engine/,/model/and use dependency injection patterns for component composition
Implement proper interfaces before concrete types
Follow Go naming conventions and the project's coding style defined in /docs/CodingConventions.md
Use mock generators: runmake generate-mocksafter interface changes
All inputs must be considered potentially byzantine; error classification is context-dependent and no code path is safe unless explicitly proven and documented
Use comprehensive error wrapping for debugging; avoidfmt.Errorf, useirrecoverablepackage for exceptions
NEVER log and continue on best effort basis; ALWAYS explicitly handle errors
Uses golangci-lint with custom configurations (.golangci.yml) and custom linters for Flow-specific conventions (struct write checking)
Files:
fvm/transactionVerifier_test.gofvm/transactionVerifier.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Unit tests should be co-located with the code they test
Follow the existing pattern of*_test.gofiles for test naming
Use fixtures for realistic test data as defined in/utils/unittest/
Files:
fvm/transactionVerifier_test.go
{crypto,fvm,ledger,storage}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Security checks for cryptographic misuse must be enforced using gosec
Files:
fvm/transactionVerifier_test.gofvm/transactionVerifier.go
{crypto,fvm,ledger,access,engine}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Cryptographic operations require careful handling; refer to crypto library documentation for proper implementation
Files:
fvm/transactionVerifier_test.gofvm/transactionVerifier.go
{storage,ledger,execution,fvm}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
State consistency is paramount; use proper synchronization primitives
Files:
fvm/transactionVerifier_test.gofvm/transactionVerifier.go
🧠 Learnings (3)
📚 Learning: 2025-12-23T00:28:41.005Z
Learnt from: CR
Repo: onflow/flow-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-23T00:28:41.005Z
Learning: Applies to {crypto,fvm,ledger,access,engine}/**/*.go : Cryptographic operations require careful handling; refer to crypto library documentation for proper implementation
Applied to files:
fvm/transactionVerifier_test.go
📚 Learning: 2025-12-23T00:28:41.005Z
Learnt from: CR
Repo: onflow/flow-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-23T00:28:41.005Z
Learning: Applies to {crypto,fvm,ledger,storage}/**/*.go : Security checks for cryptographic misuse must be enforced using gosec
Applied to files:
fvm/transactionVerifier_test.go
📚 Learning: 2025-12-23T00:28:41.005Z
Learnt from: CR
Repo: onflow/flow-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-23T00:28:41.005Z
Learning: Applies to {network,engine,consensus}/**/*.go : Network messages must be authenticated and validated
Applied to files:
fvm/transactionVerifier_test.go
🧬 Code graph analysis (2)
fvm/transactionVerifier_test.go (5)
fvm/context.go (6)
Context(32-54)NewContext(57-59)WithAuthorizationChecksEnabled(278-283)WithAccountKeyWeightThreshold(299-304)WithSequenceNumberCheckAndIncrementEnabled(287-292)WithTransactionBodyExecutionEnabled(310-315)fvm/environment/accounts.go (1)
StatefulAccounts(57-59)utils/unittest/fixtures.go (1)
RandomAddressFixture(77-79)fvm/crypto/hash.go (1)
NewPrefixedHashing(63-81)model/flow/constants.go (1)
TransactionTagString(79-79)
fvm/transactionVerifier.go (1)
model/flow/account.go (1)
RuntimeAccountPublicKey(135-142)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (37)
- GitHub Check: Lint (./)
- GitHub Check: Lint (./integration/)
- GitHub Check: Lint (./insecure/)
- GitHub Check: Unit Tests (engine/execution/ingestion)
- GitHub Check: Unit Tests (network/p2p/scoring)
- GitHub Check: Unit Tests (network/p2p/connection)
- GitHub Check: Unit Tests (network/p2p)
- GitHub Check: Unit Tests (engine/common)
- GitHub Check: Unit Tests (network/p2p/node)
- GitHub Check: Unit Tests (network)
- GitHub Check: Unit Tests (network/test/cohort1)
- GitHub Check: Unit Tests (utils)
- GitHub Check: Unit Tests (network/test/cohort2)
- GitHub Check: Unit Tests (admin)
- GitHub Check: Unit Tests (others)
- GitHub Check: Unit Tests (cmd)
- GitHub Check: Unit Tests (module)
- GitHub Check: Unit Tests (engine/execution)
- GitHub Check: Unit Tests (network/alsp)
- GitHub Check: Unit Tests (engine/verification)
- GitHub Check: Unit Tests (storage)
- GitHub Check: Unit Tests (engine/access)
- GitHub Check: Unit Tests (engine/execution/computation)
- GitHub Check: Unit Tests (engine/collection)
- GitHub Check: Unit Tests (engine)
- GitHub Check: Unit Tests (consensus)
- GitHub Check: Unit Tests (engine/consensus)
- GitHub Check: Unit Tests (module/dkg)
- GitHub Check: Unit Tests (ledger)
- GitHub Check: Unit Tests (fvm)
- GitHub Check: Unit Tests (state)
- GitHub Check: Unit Tests Insecure (insecure)
- GitHub Check: Unit Tests Insecure (insecure/integration/functional/test/gossipsub/rpc_inspector)
- GitHub Check: Unit Tests Insecure (insecure/integration/functional/test/gossipsub/scoring)
- GitHub Check: Integration Tests Others (integration)
- GitHub Check: Docker Build
- GitHub Check: analyze-code (go)
🔇 Additional comments (10)
fvm/transactionVerifier.go (4)
40-48: LGTM!The updated documentation accurately reflects the new function names and calling sequence. The comments clearly indicate that
accountKeyis set bygetAccountKeysAndAggregateWeights()and verification state is set byverifySignatures().
89-176: Well-structured signature validation and deduplication.The function correctly:
- Builds the set of allowed addresses (payer, proposer, authorizers)
- Validates each signature is from an allowed address with a clear error message
- Detects duplicate signatures by (address, keyIndex) pair
- Returns empty weight maps to be populated later
The early error returns for invalid signatures ensure fail-fast behavior.
273-278: Potentially misleading error wrapping for signature verification errors.
verifySignaturesmay return payload signature errors or envelope signature errors, but wrapping them all withNewInvalidProposalSignatureErrorcould be misleading. The inner error fromverifySignaturesalready contains the specific error type (payload vs envelope) viaentry.errorBuilder.Consider returning the error directly without re-wrapping, or verify this is intentional behavior:
err = v.verifySignatures(signatures) if err != nil { - return errors.NewInvalidProposalSignatureError(tx.ProposalKey, err) + return err }
283-321: LGTM!The function correctly:
- Fetches account public keys for each signature
- Validates keys are not revoked
- Aggregates weights into the shared maps (payloadWeights/envelopeWeights) via the
aggregateWeightspointer- Ensures at least one signature matches the proposal key
The weight aggregation on line 307 properly accumulates weights per address.
fvm/transactionVerifier_test.go (6)
21-39: Clean test helpers that reduce boilerplate.The
newContext()andnewAccount()helpers provide consistent test setup and reduce code duplication across test cases. ThefullWeightconstant clearly ties the key weight threshold to account key weights.
47-54: LGTM!The test setup creates accounts using the helper and adds a partial weight key for testing insufficient weight scenarios. The comment clearly explains the purpose of the partial weight key.
67-117: LGTM!Both duplicate signature tests correctly verify that the verifier rejects:
- Duplicate signatures within payload signatures
- Duplicate signatures across payload and envelope signatures
The tests use the
newContext()helper consistently.
119-244: Comprehensive signature validation tests.These tests effectively verify:
- Invalid envelope signatures are detected even with valid payload signatures
- Invalid payload signatures are detected even with valid envelope signatures
- The correct error type is returned (
IsInvalidEnvelopeSignatureErrorvsIsInvalidPayloadSignatureError)- Error messages include the relevant account address
The TODO on line 206-208 acknowledges a known ordering issue.
246-300: LGTM!This test correctly verifies that missing authorizer signatures result in an authorization error. The test:
- Sets up 3 authorizers but only provides valid signatures for 2
- Verifies the error mentions the authorizer (
address4) that lacks sufficient signatures
480-538: LGTM!The tag combinations test effectively verifies that signature verification enforces the correct domain tag (
flow.TransactionDomainTag). Invalid or missing tags correctly result inInvalidEnvelopeSignatureError.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@fvm/transactionVerifier.go`:
- Around line 154-159: The unrelated-account signature check in
transactionVerifier.go currently builds the error with fmt.Errorf and
entry.newError; replace this with FVM-style coded errors: either add a
NewUnrelatedAccountSignatureError in fvm/errors/txVerifier.go and use
entry.newError(WrapCodedError(NewUnrelatedAccountSignatureError(), ...)) or
remove the check entirely (since access/validator/validator.go already validates
it). Update the branch that inspects signature.Address and the return to use
WrapCodedError with a dedicated error code (similar to
ErrCodeInvalidPayloadSignatureError) so the error follows the CodedError pattern
used by other signature validations.
🧹 Nitpick comments (2)
access/validator/validator.go (1)
235-237: Metrics label may not reflect expanded validation scope.The
checkAccountsfunction now validates both duplicate signatures and unrelated account signatures, but the failure metric is stillmetrics.DuplicatedSignature. Consider adding a distinct metric for unrelated account errors to improve observability granularity.fvm/transactionVerifier.go (1)
274-279: Wrapping all signature verification errors asInvalidProposalSignatureErrormay obscure root cause.When
verifySignaturesfails, the error is wrapped withNewInvalidProposalSignatureError. However, the underlying error could be from any signature (payload or envelope), not necessarily the proposal signature. This may cause confusion during debugging.Consider returning the error directly or using a more generic wrapper that doesn't imply the proposal signature specifically failed.
Proposed fix
// Verify all cryptographic signatures against account public keys (concurrently) // and fail if at least one signature is invalid err = v.verifySignatures(signatures) if err != nil { - return errors.NewInvalidProposalSignatureError(tx.ProposalKey, err) + return err // The error is already typed (InvalidPayloadSignatureError or InvalidEnvelopeSignatureError) }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
access/validator/validator.gofvm/transactionVerifier.go
🧰 Additional context used
📓 Path-based instructions (4)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/coding_conventions.mdc)
Follow Go coding conventions as documented in
@docs/agents/CodingConventions.mdFollow Go coding standards and conventions as documented in
@docs/agents/GoDocs.md
**/*.go: Follow the existing module structure in/module/,/engine/,/model/and use dependency injection patterns for component composition
Implement proper interfaces before concrete types
Follow Go naming conventions and the project's coding style defined in /docs/CodingConventions.md
Use mock generators: runmake generate-mocksafter interface changes
All inputs must be considered potentially byzantine; error classification is context-dependent and no code path is safe unless explicitly proven and documented
Use comprehensive error wrapping for debugging; avoidfmt.Errorf, useirrecoverablepackage for exceptions
NEVER log and continue on best effort basis; ALWAYS explicitly handle errors
Uses golangci-lint with custom configurations (.golangci.yml) and custom linters for Flow-specific conventions (struct write checking)
Files:
access/validator/validator.gofvm/transactionVerifier.go
{crypto,fvm,ledger,access,engine}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Cryptographic operations require careful handling; refer to crypto library documentation for proper implementation
Files:
access/validator/validator.gofvm/transactionVerifier.go
{crypto,fvm,ledger,storage}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Security checks for cryptographic misuse must be enforced using gosec
Files:
fvm/transactionVerifier.go
{storage,ledger,execution,fvm}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
State consistency is paramount; use proper synchronization primitives
Files:
fvm/transactionVerifier.go
🧬 Code graph analysis (2)
access/validator/validator.go (2)
module/metrics/labels.go (1)
DuplicatedSignature(177-177)access/validator/errors.go (1)
UnrelatedAccountSignatureError(82-84)
fvm/transactionVerifier.go (1)
model/flow/account.go (1)
RuntimeAccountPublicKey(135-142)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (37)
- GitHub Check: Lint (./integration/)
- GitHub Check: Lint (./)
- GitHub Check: Lint (./insecure/)
- GitHub Check: Unit Tests (engine/execution/computation)
- GitHub Check: Unit Tests (network)
- GitHub Check: Unit Tests (network/alsp)
- GitHub Check: Unit Tests (network/p2p)
- GitHub Check: Unit Tests (engine/consensus)
- GitHub Check: Unit Tests (engine/common)
- GitHub Check: Unit Tests (engine/verification)
- GitHub Check: Unit Tests (network/test/cohort1)
- GitHub Check: Unit Tests (engine/execution)
- GitHub Check: Unit Tests (engine/collection)
- GitHub Check: Unit Tests (network/p2p/scoring)
- GitHub Check: Unit Tests (network/test/cohort2)
- GitHub Check: Unit Tests (module)
- GitHub Check: Unit Tests (network/p2p/node)
- GitHub Check: Unit Tests (cmd)
- GitHub Check: Unit Tests (engine/access)
- GitHub Check: Unit Tests (others)
- GitHub Check: Unit Tests (module/dkg)
- GitHub Check: Unit Tests (utils)
- GitHub Check: Unit Tests (storage)
- GitHub Check: Unit Tests (ledger)
- GitHub Check: Unit Tests (engine)
- GitHub Check: Unit Tests (network/p2p/connection)
- GitHub Check: Unit Tests (state)
- GitHub Check: Unit Tests (fvm)
- GitHub Check: Unit Tests (admin)
- GitHub Check: Unit Tests (engine/execution/ingestion)
- GitHub Check: Unit Tests (consensus)
- GitHub Check: Unit Tests Insecure (insecure)
- GitHub Check: Unit Tests Insecure (insecure/integration/functional/test/gossipsub/rpc_inspector)
- GitHub Check: Unit Tests Insecure (insecure/integration/functional/test/gossipsub/scoring)
- GitHub Check: Integration Tests Others (integration)
- GitHub Check: Docker Build
- GitHub Check: analyze-code (go)
🔇 Additional comments (7)
access/validator/validator.go (2)
415-444: LGTM! Well-structured account validation with clear separation of concerns.The function correctly:
- Checks for duplicate (address, keyIndex) pairs across both payload and envelope signatures
- Validates that all signature addresses belong to payer, proposer, or authorizers
- Returns the appropriate
UnrelatedAccountSignatureErrorfor unrelated accountsThe logic is sound and aligns with the new
UnrelatedAccountSignatureErrortype defined inerrors.go.
446-495: LGTM! Comprehensive signature format validation.The implementation correctly:
- Validates extension data for both payload and envelope signatures separately
- Checks that raw signatures are valid ECDSA signatures for either P-256 or secp256k1 curves
- Uses appropriate error types (
InvalidAuthenticationSchemeFormatError,InvalidRawSignatureError)Note: The typo "idependently" from the previous review has been fixed to "independently".
fvm/transactionVerifier.go (5)
40-48: LGTM! Comments accurately describe the verification flow.The updated comments correctly document:
accountKeyis set bygetAccountKeysAndAggregateWeights()invokedVerifyandverifyErrare set byverifySignatures()- The call order guarantee (keys populated before verification)
69-73: Early return on invalid extension data.Good addition aligning with the commit message "return early when invalid extension data". This prevents unnecessary cryptographic verification when the extension data is malformed.
90-106: LGTM! Transaction address set construction is correct.The function properly builds the set of valid transaction addresses from payer, proposer, and authorizers before validating signatures.
284-322: LGTM! Key retrieval and weight aggregation logic is correct.The function properly:
- Retrieves account public keys for each signature
- Checks for revoked keys
- Aggregates weights into the appropriate map (
payloadWeightsorenvelopeWeights) via theaggregateWeightsreference- Ensures a proposal signature exists
The weight aggregation using
signature.aggregateWeights[signature.Address] += accountKey.Weightcorrectly updates the maps passed fromnewSignatureEntries.
324-408: Concurrent verification with deterministic error propagation is well implemented.The implementation correctly handles:
- Early cancellation when any worker encounters an error
- Deterministic error output by waiting for all workers and returning the first error in signature order
- Proper cleanup with
defer cancel()andwg.Wait()One minor observation: The
panicat line 407 is a safeguard for an "impossible" state. Consider adding a comment explaining this is defensive programming.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
fvm/transactionVerifier.go (1)
179-187: Minor typo in documentation.Line 182 has a typo: "enoguh" should be "enough".
-// all accounts provides enoguh weights +// all accounts provide enough weights
♻️ Duplicate comments (1)
fvm/transactionVerifier.go (1)
154-159: Consider a dedicated error type for unrelated account signatures.The validation correctly rejects signatures from accounts not involved in the transaction. However, wrapping this error with
NewInvalidPayloadSignatureErrororNewInvalidEnvelopeSignatureErrormay be semantically misleading since the issue isn't an "invalid signature" but rather a signature from an "unrelated account."This was previously flagged - consider adding a dedicated
NewUnrelatedAccountSignatureErrorinfvm/errors/txVerifier.goto match the pattern used inaccess/validator/errors.gowhich already hasUnrelatedAccountSignatureError. This would provide clearer error categorization for clients.
🧹 Nitpick comments (2)
fvm/transactionVerifier.go (2)
286-311: LGTM with minor observation.The weight aggregation through shared map references is correct. The comment at line 309 helpfully documents this behavior.
Minor: The
_ storage.TransactionPreparerparameter at line 289 is unused. If it's kept for interface compatibility, consider adding a comment explaining why.
317-321: Consider using a CodedError for missing proposal signature.The error at lines 318-320 uses
fmt.Errorfdirectly, which is inconsistent with other signature-related errors in this file that useCodedErrortypes (e.g.,NewInvalidPayloadSignatureError,NewInvalidEnvelopeSignatureError).Per coding guidelines, prefer structured error wrapping for FVM errors. Consider whether this should return a dedicated coded error or be wrapped consistently with the signature validation pattern.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
fvm/transactionVerifier.go
🧰 Additional context used
📓 Path-based instructions (4)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/coding_conventions.mdc)
Follow Go coding conventions as documented in
@docs/agents/CodingConventions.mdFollow Go coding standards and conventions as documented in
@docs/agents/GoDocs.md
**/*.go: Follow the existing module structure in/module/,/engine/,/model/and use dependency injection patterns for component composition
Implement proper interfaces before concrete types
Follow Go naming conventions and the project's coding style defined in /docs/CodingConventions.md
Use mock generators: runmake generate-mocksafter interface changes
All inputs must be considered potentially byzantine; error classification is context-dependent and no code path is safe unless explicitly proven and documented
Use comprehensive error wrapping for debugging; avoidfmt.Errorf, useirrecoverablepackage for exceptions
NEVER log and continue on best effort basis; ALWAYS explicitly handle errors
Uses golangci-lint with custom configurations (.golangci.yml) and custom linters for Flow-specific conventions (struct write checking)
Files:
fvm/transactionVerifier.go
{crypto,fvm,ledger,storage}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Security checks for cryptographic misuse must be enforced using gosec
Files:
fvm/transactionVerifier.go
{crypto,fvm,ledger,access,engine}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Cryptographic operations require careful handling; refer to crypto library documentation for proper implementation
Files:
fvm/transactionVerifier.go
{storage,ledger,execution,fvm}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
State consistency is paramount; use proper synchronization primitives
Files:
fvm/transactionVerifier.go
🧠 Learnings (4)
📚 Learning: 2025-12-23T00:28:41.005Z
Learnt from: CR
Repo: onflow/flow-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-23T00:28:41.005Z
Learning: Applies to {crypto,fvm,ledger,access,engine}/**/*.go : Cryptographic operations require careful handling; refer to crypto library documentation for proper implementation
Applied to files:
fvm/transactionVerifier.go
📚 Learning: 2025-12-23T00:28:41.005Z
Learnt from: CR
Repo: onflow/flow-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-23T00:28:41.005Z
Learning: Applies to {crypto,fvm,ledger,storage}/**/*.go : Security checks for cryptographic misuse must be enforced using gosec
Applied to files:
fvm/transactionVerifier.go
📚 Learning: 2025-12-23T00:28:41.005Z
Learnt from: CR
Repo: onflow/flow-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-23T00:28:41.005Z
Learning: Applies to **/*.go : All inputs must be considered potentially byzantine; error classification is context-dependent and no code path is safe unless explicitly proven and documented
Applied to files:
fvm/transactionVerifier.go
📚 Learning: 2025-12-23T00:28:41.005Z
Learnt from: CR
Repo: onflow/flow-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-23T00:28:41.005Z
Learning: Applies to {storage,ledger,execution,fvm}/**/*.go : State consistency is paramount; use proper synchronization primitives
Applied to files:
fvm/transactionVerifier.go
🧬 Code graph analysis (1)
fvm/transactionVerifier.go (1)
model/flow/account.go (1)
RuntimeAccountPublicKey(135-142)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (37)
- GitHub Check: Lint (./integration/)
- GitHub Check: Lint (./insecure/)
- GitHub Check: Lint (./)
- GitHub Check: Unit Tests (network/test/cohort1)
- GitHub Check: Unit Tests (network/p2p/connection)
- GitHub Check: Unit Tests (network/alsp)
- GitHub Check: Unit Tests (network/p2p/node)
- GitHub Check: Unit Tests (engine/execution)
- GitHub Check: Unit Tests (engine/execution/ingestion)
- GitHub Check: Unit Tests (engine/collection)
- GitHub Check: Unit Tests (module/dkg)
- GitHub Check: Unit Tests (network)
- GitHub Check: Unit Tests (engine/verification)
- GitHub Check: Unit Tests (utils)
- GitHub Check: Unit Tests (engine)
- GitHub Check: Unit Tests (engine/common)
- GitHub Check: Unit Tests (storage)
- GitHub Check: Unit Tests (module)
- GitHub Check: Unit Tests (others)
- GitHub Check: Unit Tests (engine/consensus)
- GitHub Check: Unit Tests (network/p2p)
- GitHub Check: Unit Tests (engine/execution/computation)
- GitHub Check: Unit Tests (network/test/cohort2)
- GitHub Check: Unit Tests (engine/access)
- GitHub Check: Unit Tests (network/p2p/scoring)
- GitHub Check: Unit Tests (admin)
- GitHub Check: Unit Tests (state)
- GitHub Check: Unit Tests (cmd)
- GitHub Check: Unit Tests (fvm)
- GitHub Check: Unit Tests (ledger)
- GitHub Check: Unit Tests (consensus)
- GitHub Check: Unit Tests Insecure (insecure/integration/functional/test/gossipsub/rpc_inspector)
- GitHub Check: Unit Tests Insecure (insecure/integration/functional/test/gossipsub/scoring)
- GitHub Check: Unit Tests Insecure (insecure)
- GitHub Check: Integration Tests Others (integration)
- GitHub Check: Docker Build
- GitHub Check: analyze-code (go)
🔇 Additional comments (6)
fvm/transactionVerifier.go (6)
33-49: LGTM!The updated comments accurately describe the lifecycle and data flow of
signatureContinuation. The struct design cleanly separates initial input, key retrieval state, and verification state.
62-88: LGTM!The memoization pattern for concurrent signature verification is correctly implemented. The early-exit on invalid extension data prevents unnecessary cryptographic operations.
139-174: LGTM!The deduplication logic correctly identifies duplicate signatures using the (address, keyIndex) pair and fails fast with a clear error message.
326-410: LGTM!The concurrent verification implementation is well-designed:
- Worker pool pattern with configurable concurrency
- Early cancellation via context on first error (performance optimization)
- Deterministic error ordering by iterating
signaturesslice afterwg.Wait()ensures reproducible behavior across executionsThe defensive panic at line 409 is appropriate as a programming-error safeguard that should never be reached if the logic is correct.
207-284: Good defense-in-depth approach.The verification flow correctly implements fail-fast semantics:
- Validate and deduplicate signatures (cheap)
- Retrieve account keys and aggregate weights (single pass)
- Check weight thresholds (cheap)
- Cryptographic verification (expensive) - only if all prior checks pass
This ordering ensures expensive crypto operations are avoided when cheaper validation fails, which is appropriate given that all inputs must be considered potentially byzantine. Based on learnings.
274-281: No concern - error wrapping correctly preserves signature error types.The error framework is designed to preserve coded error information through wrapping chains.
NewInvalidPayloadSignatureErrorandNewInvalidEnvelopeSignatureErrorcreate distinctCodedErrorinstances with different error codes (1008 and 1009). When wrapped byNewInvalidProposalSignatureErrorat line 280 usingWrapCodedError, the underlying error chain is preserved viafmt.Errorf("%s: %w", ...). TheFind()function recursively unwraps errors to locate anyCodedErrormatching a specific code anywhere in the chain, so the distinction between payload and envelope signature errors remains accessible and detectable throughHasErrorCodeand related functions.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| // Checks related to the accounts used in the transaction: | ||
| // - no duplicate account keys signing | ||
| // - no unrelated account signatures (from accounts that are neither proposer, payer, nor authorizers) |
There was a problem hiding this comment.
| // Checks related to the accounts used in the transaction: | |
| // - no duplicate account keys signing | |
| // - no unrelated account signatures (from accounts that are neither proposer, payer, nor authorizers) | |
| // checkAccounts validates accounts used in the transaction | |
| // - no duplicate account keys signing | |
| // - no unrelated account signatures (from accounts that are neither proposer, payer, nor authorizers) | |
| // | |
| // Note: This function does not verify that each required role (payer, proposer, authorizers) has | |
| // provided sufficient signatures. The FVM already handles this with full weight verification | |
| // (see fvm/transactionVerifier.go). The access validator is intentionally lightweight since it | |
| // lacks access to account keys/weights. |
There was a problem hiding this comment.
Actually, I wonder if make sense to add some light weight check as below, so that access/collection nodes can reject them earlier. Execution node still has to read the execution state for the keys/weights.
- Payer: must have ≥1 envelope signature
- Proposer: must have ≥1 signature in payload OR envelope
- Authorizers: if authorizer == payer, skip (they sign envelope only). Otherwise, must have ≥1 payload signature.
There was a problem hiding this comment.
Good idea to reject at the access level, add it here: 3ec9e4b
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@access/validator/validator.go`:
- Around line 434-450: The proposer check currently accepts any signature from
the proposer address; update the logic in validator.go (around
tx.EnvelopeSignatures/tx.PayloadSignatures handling and the proposer check) to
require a signature that matches both tx.ProposalKey.Address and
tx.ProposalKey.KeyIndex. Instead of recording only
observedEnvelopeSig[addr]=true and observedPayloadSig[addr]=true, track observed
key indices per address (or iterate signatures) and verify that there exists an
envelope or payload signature whose Address equals tx.ProposalKey.Address and
KeyIndex equals tx.ProposalKey.KeyIndex; if none found, return
MissingSignatureError{Address: tx.ProposalKey.Address, Message: "proposer
signature on either payload or envelope is missing"}.
In `@engine/collection/ingest/engine_test.go`:
- Around line 322-336: The test "unrelated signature (payload only)" fails early
because no payer envelope signature is added; modify the test setup in this case
to append a payer envelope signature to tx.EnvelopeSignatures (e.g. using
unittest.TransactionSignatureFixture()) with Address set to signer but a
different KeyIndex than any existing signature (to avoid duplicate-signature
errors), while keeping tx.PayloadSignatures as sig1 and unrelatedSig so
checkAccounts proceeds to the unrelated-signature validation.
🧹 Nitpick comments (1)
access/validator/validator.go (1)
235-236: Metrics label now conflates multiple error types.
checkAccountscan return missing/unrelated signature errors, but the failure metric is stillmetrics.DuplicatedSignature. Consider adding distinct labels or splitting checks so telemetry reflects the actual failure reason.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@integration/internal/emulator/tests/accounts_test.go`:
- Around line 995-1000: The test currently signs the payload with
tx.SignPayload(accountAddressB, 0, signerB) then appends a new signature with
tx.AddPayloadSignature(accountAddressB, 0, []byte{1,2,3}), which triggers the
verifier's duplicate-signature check instead of testing signature validation;
modify the test to mutate/corrupt the existing payload signature produced by
tx.SignPayload (e.g., flip a byte in the signature slice stored on the tx
object) rather than calling tx.AddPayloadSignature so the verifier sees a single
signature for (accountAddressB, keyIndex 0) with invalid contents and the test
becomes deterministic. Ensure you locate the signature produced by
tx.SignPayload in the tx structure and alter it in-place before sending the
transaction.
🧹 Nitpick comments (1)
utils/unittest/fixtures/transaction.go (1)
89-100: Keep payer/authorizer defaults aligned when ProposalKey is overridden.Line 89-99 sets payer/authorizers from the initially generated proposal key. If a caller uses
WithProposalKeyonly, the “same address” invariant breaks and you end up with a proposer/payer mismatch. Consider re-aligning payer/authorizers after options when they’re still the initial defaults.♻️ Suggested adjustment
proposalKey := g.proposalKeys.Fixture() proposalAddress := proposalKey.Address tx := &flow.TransactionBody{ ReferenceBlockID: g.identifiers.Fixture(), Script: []byte("access(all) fun main() {}"), GasLimit: 10, ProposalKey: proposalKey, Payer: proposalAddress, Authorizers: []flow.Address{proposalAddress}, } for _, opt := range opts { opt(g, tx) } + + // If only ProposalKey was overridden, keep the "single address" invariant. + if tx.ProposalKey.Address != proposalAddress && + tx.Payer == proposalAddress && + len(tx.Authorizers) == 1 && + tx.Authorizers[0] == proposalAddress { + tx.Payer = tx.ProposalKey.Address + tx.Authorizers = []flow.Address{tx.ProposalKey.Address} + }Also applies to: 106-114
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
integration/tests/execution/failing_tx_reverted_test.go (1)
65-77: Update the envelope signature'sKeyIndexto 1 and correct the comment.The comment claims "no sigs," but the code adds an envelope signature. More importantly, the signature omits
KeyIndex, which defaults to 0. However,ProposalKeyFixture()usesKeyIndex: 1, and both the payer and proposal key are the same service account. WithKeyIndex: 0, the signature won't match the account's key index and will fail validation for the wrong reason. SetKeyIndex: 1on the envelope signature to properly test missing payload signatures, and update the comment to reflect that an invalid envelope signature is intentionally added.Current code
// send transaction that has no sigs and should not execute failingTx = lib.SDKTransactionFixture( lib.WithTransactionDSL(lib.CreateCounterTx(sdk.Address(serviceAddress))), lib.WithReferenceBlock(sdk.Identifier(s.net.Root().ID())), lib.WithChainID(chainID), ) failingTx.PayloadSignatures = nil failingTx.EnvelopeSignatures = []sdk.TransactionSignature{ sdk.TransactionSignature{ Address: failingTx.Payer, Signature: unittest.SignatureFixtureForTransactions(), }, }
| t.Parallel() | ||
|
|
||
| t.Run("Missing payload signature", func(t *testing.T) { | ||
|
|
There was a problem hiding this comment.
This test is not possible behind the access API anymore. It is replaced by this direct FVM test:
flow-go/fvm/transactionVerifier_test.go
Line 246 in 976eb30
| t.Parallel() | ||
|
|
||
| t.Run("Missing envelope signature", func(t *testing.T) { | ||
|
|
There was a problem hiding this comment.
This test is not possible behind the access API anymore. It is replaced by this direct FVM test:
https://github.com/onflow/flow-go/blob/tarak/fvm-tx-verifier/fvm/transactionVerifier_test.go#L393
addresses #8277