Conversation
- Simplified the comment in the attestation schema migration by removing the composite primary key reference for the attestations table. - Removed the detailed comment block in the attestation actions migration, streamlining the file for better readability. These changes enhance the clarity and maintainability of the migration files.
…ttestations - Introduced the tn_attestation extension, which includes functionality for signing attestation payloads using a validator's key, processing attestation hashes, and managing the attestation queue. - Added methods for preparing signing work, submitting signatures, and handling attestation lifecycle events. - Implemented comprehensive tests to ensure the reliability of the signing workflow and attestation processing. - Enhanced the integration with the existing system by ensuring proper logging and error handling throughout the attestation process. These changes improve the overall functionality and maintainability of the attestation system, enabling efficient signing and processing of attestations in the Kwil framework.
WalkthroughAdds a tn_attestation extension that parses canonical payloads, prepares and signs attestations (leader-only), broadcasts sign_attestation transactions via a JSON‑RPC broadcaster, monitors tx status asynchronously, updates migrations/actions, and includes extensive unit and integration tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Node
participant Ext as tn_attestation
participant DB
participant Signer
participant Broadcaster
participant StatusQ as Status Worker
Note over Node,Ext: EndBlock (leader only)
Node->>Ext: OnEndBlock(height)
alt In-memory queue non-empty
Ext->>Ext: dequeue hashes
else Fallback scan window reached
Ext->>DB: fetchPendingHashes(limit)
DB-->>Ext: [attestation_hash...]
end
loop For each hash
Ext->>DB: fetchUnsignedAttestations(hash)
DB-->>Ext: records (payload, requester, created_height)
Ext->>Signer: SignDigest(SHA256(payload.SigningBytes()))
Signer-->>Ext: 65-byte signature
Ext->>Broadcaster: BroadcastTx(sign_attestation(hash, requester, height, sig))
Broadcaster-->>Ext: txHash (+optional TxResult)
Ext->>StatusQ: enqueueStatusCheck(txHash, hash, requester)
end
par Async status monitoring
StatusQ->>Broadcaster: TxQuery(txHash) (retries/backoff)
Broadcaster-->>StatusQ: TxQueryResponse
StatusQ-->>Ext: log outcome
end
sequenceDiagram
autonumber
participant Ext as tn_attestation
participant RPC as JSON-RPC
participant Pool as Conn Pool
Note over Ext,RPC: Broadcaster initialization
Ext->>Ext: normalizeListenAddressForClient(config.rpc_url || node RPC)
Ext->>Pool: makeBroadcasterFromURL(url) (shared client/pool)
Ext->>RPC: broadcast_tx(sync=BroadcastWaitAccept|BroadcastWaitCommit)
alt sync=BroadcastWaitCommit (blocking)
Ext->>RPC: tx_query(txHash)
RPC-->>Ext: TxQueryResponse
else sync=BroadcastWaitAccept (fast)
RPC-->>Ext: txHash
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
…dations - Removed redundant length checks for attestation_hash, requester, and signature in the SQL action to simplify validation logic. - Enhanced comments in the integration test to clarify the workflow being tested, focusing on the complete production signing process. - Adjusted the test setup to ensure the tn_attestation precompile is registered correctly for testing. These changes improve the clarity and maintainability of the integration tests and SQL action validations, ensuring a more robust attestation signing workflow.
- Introduced a new doc.go file for the tn_attestation package, detailing the attestation signing workflow and key components. - Added a comment to the constants.go file to clarify the purpose of the ExtensionName constant. These changes improve the documentation and clarity of the tn_attestation extension, aiding developers in understanding its functionality and usage.
…and transaction querying - Removed the txBroadcasterFunc and replaced it with a jsonRPCBroadcaster for better clarity and functionality. - Updated the ensureBroadcaster method to set both the broadcaster and transaction query client. - Introduced a new method to check transaction status asynchronously, improving logging and error handling. - Enhanced the submitSignature method to use non-blocking transaction broadcasting and added logging for transaction status. These changes improve the maintainability and functionality of the tn_attestation extension, ensuring efficient transaction handling and better integration with the signing workflow.
…ension - Introduced a dedicated background worker for asynchronous transaction status checks, improving responsiveness and logging. - Replaced blocking transaction status checks with a queuing mechanism to prevent interference with consensus processing. - Updated methods to streamline transaction broadcasting and status querying, ensuring better integration with the signing workflow. These changes enhance the efficiency and maintainability of the tn_attestation extension, providing improved operational visibility for transaction confirmations.
…extension - Introduced a new test file for the status worker, implementing tests to verify successful transaction processing and retry logic on failure. - Created a fake transaction query client to simulate responses and errors, ensuring robust testing of the status worker's behavior. - Enhanced the testing framework to validate the asynchronous handling of transaction status checks. These changes improve the test coverage and reliability of the tn_attestation extension, ensuring proper functionality of the transaction status monitoring system.
…orker_test.go - Added comments to the newFakeTxQueryClient function to clarify its purpose and usage, including details on expected parameters and behavior. - This enhancement improves code readability and aids future developers in understanding the testing framework for the transaction status worker. These changes contribute to better documentation practices within the tn_attestation extension's test suite.
- Added detailed comments in processor.go to explain the handling of unsigned attestations and the importance of hash verification during signing. - Improved comments in worker.go regarding nonce handling for signatures and the resilience of the status worker across leader transitions. These changes enhance code readability and provide better context for future developers working on the tn_attestation extension.
![coderabbitai[bot]](https://waps.l3s.uni-hannover.de/live/im_/https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
extensions/tn_attestation/worker.go (1)
115-126: Handle account lookup “not found” with typed errors, not string matchingComparing
err.Error()text for"not found"/"no rows"is brittle and can silently regress if upstream error messages change. Please switch toerrors.Is(e.g. againstsql.ErrNoRowsor a dedicated sentinel returned byAccounts.GetAccount) or expose an explicit helper on the accounts subsystem so the transport isn’t coupled to error strings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
extensions/tn_attestation/broadcast.go(1 hunks)extensions/tn_attestation/canonical.go(1 hunks)extensions/tn_attestation/canonical_test.go(1 hunks)extensions/tn_attestation/constants.go(1 hunks)extensions/tn_attestation/doc.go(1 hunks)extensions/tn_attestation/extension.go(1 hunks)extensions/tn_attestation/harness_integration_test.go(1 hunks)extensions/tn_attestation/integration_test.go(1 hunks)extensions/tn_attestation/precompile.go(1 hunks)extensions/tn_attestation/processor.go(1 hunks)extensions/tn_attestation/processor_test.go(1 hunks)extensions/tn_attestation/signer.go(2 hunks)extensions/tn_attestation/signer_test.go(10 hunks)extensions/tn_attestation/status_worker_test.go(1 hunks)extensions/tn_attestation/tn_attestation.go(7 hunks)extensions/tn_attestation/tn_attestation_test.go(2 hunks)extensions/tn_attestation/worker.go(1 hunks)internal/migrations/023-attestation-schema.sql(1 hunks)internal/migrations/024-attestation-actions.sql(1 hunks)tests/streams/attestation/attestation_request_test.go(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-10-10T13:00:13.658Z
Learnt from: outerlook
PR: trufnetwork/node#1207
File: internal/migrations/023-attestation-schema.sql:20-21
Timestamp: 2025-10-10T13:00:13.658Z
Learning: In the attestations table (internal/migrations/023-attestation-schema.sql), the primary key is (requester, created_height, attestation_hash) because attestation_hash is computed deterministically from user input only (version|algo|data_provider|stream_id|action_id|args) and does not include created_height. This allows the same user to request the same attestation at different block heights.
Applied to files:
internal/migrations/023-attestation-schema.sql
📚 Learning: 2025-08-20T12:26:14.229Z
Learnt from: outerlook
PR: trufnetwork/node#1113
File: extensions/tn_digest/extension.go:64-65
Timestamp: 2025-08-20T12:26:14.229Z
Learning: In the tn_digest extension singleton pattern, SetExtension uses direct assignment (not sync.Once) to allow test dependency injection and mock overrides after GetExtension has been called.
Applied to files:
extensions/tn_attestation/extension.go
📚 Learning: 2025-10-10T13:00:14.170Z
Learnt from: outerlook
PR: trufnetwork/node#1207
File: internal/migrations/024-attestation-actions.sql:58-76
Timestamp: 2025-10-10T13:00:14.170Z
Learning: In the attestation system for internal/migrations/024-attestation-actions.sql, the attestation_hash is computed from (version|algo|data_provider|stream_id|action_id|args) and intentionally excludes created_height. This design ensures the hash is deterministic based only on user input, not network state like block height.
Applied to files:
internal/migrations/024-attestation-actions.sql
🧬 Code graph analysis (10)
extensions/tn_attestation/signer_test.go (1)
extensions/tn_attestation/signer.go (1)
NewValidatorSigner(26-43)
extensions/tn_attestation/processor.go (2)
extensions/tn_attestation/canonical.go (2)
CanonicalPayload(24-35)ParseCanonicalPayload(40-82)extensions/tn_attestation/signer.go (1)
GetValidatorSigner(122-124)
extensions/tn_attestation/tn_attestation_test.go (2)
extensions/tn_attestation/extension.go (1)
SetExtension(61-63)extensions/tn_attestation/queue.go (1)
GetAttestationQueue(115-120)
extensions/tn_attestation/canonical_test.go (1)
extensions/tn_attestation/canonical.go (1)
ParseCanonicalPayload(40-82)
extensions/tn_attestation/worker.go (1)
extensions/tn_attestation/processor.go (1)
PreparedSignature(25-32)
extensions/tn_attestation/extension.go (1)
extensions/tn_attestation/broadcast.go (2)
TxBroadcaster(125-127)TxQueryClient(130-132)
extensions/tn_attestation/processor_test.go (2)
extensions/tn_attestation/canonical.go (2)
CanonicalPayload(24-35)ParseCanonicalPayload(40-82)extensions/tn_attestation/signer.go (2)
ResetValidatorSignerForTesting(128-131)InitializeValidatorSigner(112-118)
extensions/tn_attestation/integration_test.go (4)
extensions/tn_attestation/signer.go (2)
InitializeValidatorSigner(112-118)ResetValidatorSignerForTesting(128-131)extensions/tn_attestation/canonical.go (1)
ParseCanonicalPayload(40-82)extensions/tn_attestation/queue.go (1)
GetAttestationQueue(115-120)extensions/tn_attestation/extension.go (1)
SetExtension(61-63)
extensions/tn_attestation/tn_attestation.go (1)
extensions/tn_attestation/queue.go (1)
GetAttestationQueue(115-120)
extensions/tn_attestation/harness_integration_test.go (8)
extensions/tn_attestation/constants.go (1)
ExtensionName(4-4)extensions/tn_cache/tn_cache.go (1)
InitializeCachePrecompile(102-229)extensions/database-size/extension.go (1)
InitializeDatabaseSizePrecompile(19-68)extensions/tn_attestation/tn_attestation.go (1)
InitializeExtension(20-39)internal/migrations/migration.go (1)
GetSeedScriptPaths(15-48)extensions/tn_utils/serialization.go (1)
EncodeActionArgs(45-76)extensions/tn_attestation/canonical.go (1)
ParseCanonicalPayload(40-82)extensions/tn_attestation/signer.go (3)
ResetValidatorSignerForTesting(128-131)InitializeValidatorSigner(112-118)GetValidatorSigner(122-124)
⏰ 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). (1)
- GitHub Check: acceptance-test
🔇 Additional comments (9)
internal/migrations/023-attestation-schema.sql (1)
5-5: LGTM! Documentation clarity improved.The updated comment provides a clearer, more concise description of the attestations table's purpose.
internal/migrations/024-attestation-actions.sql (2)
90-93: LGTM! Formatting improved.The updated formatting improves readability without changing functionality.
97-157: LGTM! Leader-only signing action correctly implemented.The
sign_attestationaction properly enforces:
- Leader-only access via
@leader_senderand@signervalidation- Comprehensive input validation for all required parameters
- Attestation existence and duplicate signature prevention
- Atomic signature recording with validator identity and height
The composite key constraint in the UPDATE statement correctly matches the primary key definition from migration 023.
extensions/tn_attestation/doc.go (1)
1-15: LGTM! Clear and comprehensive package documentation.The documentation effectively describes the attestation signing workflow, key components, and initialization requirements. This will help developers understand the extension's purpose and architecture.
extensions/tn_attestation/constants.go (1)
3-4: LGTM! Clean constant definition.The
ExtensionNameconstant provides a clear, centralized identifier for the attestation extension.tests/streams/attestation/attestation_request_test.go (1)
144-178: LGTM! Test correctly updated for requester field.The test updates properly handle the new
requesterfield:
- Struct definition includes the new field
- SELECT statement retrieves the requester column
- Column mapping correctly aligns with the query result order
The index adjustments are accurate and maintain proper data extraction.
extensions/tn_attestation/signer.go (2)
45-66: LGTM! SignDigest correctly implements EVM-compatible signing.The
SignDigestmethod provides a clean API for signing pre-hashed 32-byte digests:
- Thread-safe access via RLock/RUnlock
- Validates digest length matches
crypto.DigestLength(32 bytes)- Adjusts V byte to EVM standard (27/28) for compatibility with Solidity's
ecrecoverThis enables flexible signing workflows where the caller controls the hash algorithm.
68-83: LGTM! SignKeccak256 refactored for code reuse.The refactoring delegates core signing logic to
SignDigest, eliminating code duplication while maintaining the existing API surface. This is a clean separation of concerns between hash computation and signature generation.extensions/tn_attestation/signer_test.go (1)
34-350: LGTM! Comprehensive test coverage for SignDigest.The test suite properly covers the new
SignDigestAPI:
- Happy path: Valid digest signing with SHA-256 (lines 34-47)
- Error handling: Invalid digest length rejection (lines 49-60)
- Signature verification: Recovery and ECDSA verification using digests (lines 94-127)
- Deterministic behavior: Same digest produces same signature (lines 157-176)
- Thread safety: Concurrent signing correctness (lines 178-207)
- EVM compatibility: V byte validation and
ecrecoverrecovery (lines 281-350)All tests correctly use SHA-256 digests and validate the 65-byte signature format.
Time Submission Status
|
…sion - Updated the buildCanonical function to use Little Endian for length-prefixed data, improving consistency in payload generation. - Simplified the computeAttestationHash function to directly utilize the raw payload when available, enhancing performance. - Refactored the writeLengthPrefixed function for better clarity and reusability across the codebase. These changes enhance the efficiency and maintainability of the tn_attestation extension, ensuring more reliable payload processing and hashing.
- Refactored the TestComputeAttestationHash function to improve clarity and structure. - Added tests to verify behavior when the raw payload is present and when it is missing, ensuring accurate hash computation. - Utilized a canonical payload builder for consistent test data generation. These changes improve test coverage and reliability for the attestation hash computation in the tn_attestation extension.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
extensions/tn_attestation/processor_test.go (2)
185-239: Refactor duplicated argument decoding into helper functions.The four argument decoding blocks (hash, requester, created_height, signature) follow nearly identical patterns with type switching and nil checks, violating DRY principles. This makes the test harder to maintain and read.
Consider extracting into generic helpers:
func decodeBytesArg(t *testing.T, arg *ktypes.EncodedValue, fieldName string) []byte { val, err := arg.Decode() require.NoError(t, err) switch typed := val.(type) { case []byte: return typed case *[]byte: require.NotNil(t, typed, "%s argument pointer was nil", fieldName) return *typed default: t.Fatalf("unexpected %s argument type %T", fieldName, val) return nil } } func decodeInt64Arg(t *testing.T, arg *ktypes.EncodedValue, fieldName string) int64 { val, err := arg.Decode() require.NoError(t, err) switch typed := val.(type) { case int64: return typed case *int64: require.NotNil(t, typed, "%s argument pointer was nil", fieldName) return *typed default: t.Fatalf("unexpected %s argument type %T", fieldName, val) return 0 } }Then simplify the test to:
- hashVal, err := decoded.Arguments[0][0].Decode() - require.NoError(t, err) - var hashBytes []byte - switch typed := hashVal.(type) { - case []byte: - hashBytes = typed - case *[]byte: - require.NotNil(t, typed, "hash argument pointer was nil") - hashBytes = *typed - default: - t.Fatalf("unexpected hash argument type %T", hashVal) - } + hashBytes := decodeBytesArg(t, decoded.Arguments[0][0], "hash") assert.Equal(t, hash[:], hashBytes) - requesterVal, err := decoded.Arguments[0][1].Decode() - require.NoError(t, err) - var requesterBytes []byte - switch typed := requesterVal.(type) { - case []byte: - requesterBytes = typed - case *[]byte: - require.NotNil(t, typed, "requester argument pointer was nil") - requesterBytes = *typed - default: - t.Fatalf("unexpected requester argument type %T", requesterVal) - } + requesterBytes := decodeBytesArg(t, decoded.Arguments[0][1], "requester") assert.Equal(t, []byte("requester"), requesterBytes) - heightVal, err := decoded.Arguments[0][2].Decode() - require.NoError(t, err) - var createdHeight int64 - switch typed := heightVal.(type) { - case int64: - createdHeight = typed - case *int64: - require.NotNil(t, typed, "created_height argument pointer was nil") - createdHeight = *typed - default: - t.Fatalf("unexpected created_height argument type %T", heightVal) - } + createdHeight := decodeInt64Arg(t, decoded.Arguments[0][2], "created_height") assert.Equal(t, int64(123), createdHeight) - sigVal, err := decoded.Arguments[0][3].Decode() - require.NoError(t, err) - var signatureBytes []byte - switch typed := sigVal.(type) { - case []byte: - signatureBytes = typed - case *[]byte: - require.NotNil(t, typed, "signature argument pointer was nil") - signatureBytes = *typed - default: - t.Fatalf("unexpected signature argument type %T", sigVal) - } + signatureBytes := decodeBytesArg(t, decoded.Arguments[0][3], "signature") assert.Len(t, signatureBytes, 65)
288-306: Consider more robust SQL detection.The string matching on line 290 (
strings.Contains(statement, "GROUP BY attestation_hash")) is brittle and could break if the production SQL is reformatted or refactored. While acceptable for test code, a more resilient approach would be to match on statement structure or use a query identifier parameter.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
extensions/tn_attestation/canonical_test.go(1 hunks)extensions/tn_attestation/processor.go(1 hunks)extensions/tn_attestation/processor_test.go(1 hunks)extensions/tn_attestation/worker.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- extensions/tn_attestation/processor.go
- extensions/tn_attestation/canonical_test.go
🧰 Additional context used
🧬 Code graph analysis (2)
extensions/tn_attestation/worker.go (2)
extensions/tn_attestation/processor.go (1)
PreparedSignature(25-32)deployments/infra/config/node/values.go (1)
GenesisConfig(13-16)
extensions/tn_attestation/processor_test.go (2)
extensions/tn_attestation/canonical.go (1)
ParseCanonicalPayload(40-82)extensions/tn_attestation/signer.go (2)
ResetValidatorSignerForTesting(128-131)InitializeValidatorSigner(112-118)
⏰ 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). (2)
- GitHub Check: acceptance-test
- GitHub Check: lint
🔇 Additional comments (7)
extensions/tn_attestation/processor_test.go (3)
23-51: LGTM! Previous issue resolved.The test now correctly hashes the canonical bytes produced by
buildCanonical(which includes block height, result, and length prefixes), addressing the prior review concern about truncated payloads. Both test cases (hashing withrawpresent and re-encoding whenrawis missing) provide good coverage.
53-106: LGTM!The test setup is comprehensive, with proper cleanup registration, realistic canonical payload construction, and thorough assertions covering all
PreparedSignaturefields.
242-269: LGTM!The test provides good coverage of batch-limited and unlimited fetch scenarios with clear, minimal setup.
extensions/tn_attestation/worker.go (4)
32-69: LGTM: Clean batch processing with error isolation.The per-item error handling ensures that a single malformed record doesn't block the entire batch, which is essential for making steady progress. The test override hook is a thoughtful addition for unit testing.
71-174: LGTM: Thorough validation and correct nonce handling.The nonce logic correctly handles the first-ever signature case where the account doesn't exist yet (lines 112-124), and the comprehensive component validation prevents runtime panics. Broadcasting with mode 0 appropriately avoids blocking consensus, and the async status check decouples confirmation from the signing workflow.
176-213: LGTM: Well-designed worker lifecycle with defensive patterns.The
sync.Onceguard prevents goroutine leaks across leader transitions, and the non-blocking enqueue (lines 206-212) prevents EndBlock stalls if the queue is full. The defensive copy of the requester slice on line 204 is excellent practice to prevent aliasing bugs.
225-304: LGTM: Past review comment addressed, robust retry strategy.The terminal warning on lines 299-303 correctly addresses the past review comment by logging when transaction status remains unresolved after all retry attempts. The exponential backoff strategy (~115s total) is appropriate for handling block production variance, and the context cancellation support prevents runaway polling.
- Simplified the TestSubmitSignature function by introducing helper functions to decode byte and int64 arguments, enhancing readability and reducing code duplication. - Updated assertions to directly compare decoded values, ensuring clearer test expectations and error messages. - Improved error handling in the decoding process to provide more informative feedback on unexpected argument types. These changes enhance the maintainability and clarity of the tests in the tn_attestation extension, ensuring more reliable validation of signature submissions.
- Refactored the computeAttestationHash function to utilize a hasher for better performance and clarity. - Introduced a new helper function, buildHashMaterial, to streamline the construction of hash input data. - Updated tests in processor_test.go to reflect changes in hash computation logic, ensuring accurate validation of expected outputs. These changes enhance the efficiency and maintainability of the tn_attestation extension, improving the reliability of attestation hash computations.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
extensions/tn_attestation/processor.go (2)
57-62: Harden row value type handling to avoid panicsUn-guarded type assertions on requester and created_height can panic if driver types vary (e.g., TEXT string for requester). Use a type switch.
- rec := attestationRecord{ - hash: bytesClone(row.Values[0].([]byte)), - requester: bytesCloneOrNil(row.Values[1]), - canonical: bytesClone(row.Values[2].([]byte)), - createdHeight: row.Values[3].(int64), - } + // hash + hb, ok := row.Values[0].([]byte) + if !ok { + return fmt.Errorf("unexpected attestation_hash type %T", row.Values[0]) + } + // requester (nullable, may be []byte or string) + rb := bytesCloneOrNil(row.Values[1]) + // canonical + cb, ok := row.Values[2].([]byte) + if !ok { + return fmt.Errorf("unexpected result_canonical type %T", row.Values[2]) + } + // created_height + ch, ok := row.Values[3].(int64) + if !ok { + return fmt.Errorf("unexpected created_height type %T", row.Values[3]) + } + rec := attestationRecord{ + hash: bytesClone(hb), + requester: rb, + canonical: bytesClone(cb), + createdHeight: ch, + } @@ -func bytesCloneOrNil(v any) []byte { - if v == nil { - return nil - } - return bytes.Clone(v.([]byte)) -} +func bytesCloneOrNil(v any) []byte { + if v == nil { + return nil + } + switch t := v.(type) { + case []byte: + return bytes.Clone(t) + case string: + return []byte(t) + default: + return nil + } +}Please confirm the actual driver types for requester/created_height so we can tighten further. Based on learnings.
Also applies to: 195-200
146-152: Optional: reduce race on multi-leader transitions with SKIP LOCKEDIf there's any chance of concurrent leaders/processes, consider selecting hashes with row locking to avoid duplicate work.
- Option: SELECT ... FROM attestations WHERE signature IS NULL GROUP BY attestation_hash ORDER BY MIN(created_height) ASC LIMIT $limit FOR UPDATE SKIP LOCKED (adjust per engine support).
- Alternatively, insert hashes into a dedicated work queue table and claim with SKIP LOCKED.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
extensions/tn_attestation/processor.go(1 hunks)extensions/tn_attestation/processor_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- extensions/tn_attestation/processor_test.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-10T13:00:14.189Z
Learnt from: outerlook
PR: trufnetwork/node#1207
File: internal/migrations/024-attestation-actions.sql:58-76
Timestamp: 2025-10-10T13:00:14.189Z
Learning: In the attestation system for internal/migrations/024-attestation-actions.sql, the attestation_hash is computed from (version|algo|data_provider|stream_id|action_id|args) and intentionally excludes created_height. This design ensures the hash is deterministic based only on user input, not network state like block height.
Applied to files:
extensions/tn_attestation/processor.go
🧬 Code graph analysis (1)
extensions/tn_attestation/processor.go (2)
extensions/tn_attestation/canonical.go (2)
CanonicalPayload(24-35)ParseCanonicalPayload(40-82)extensions/tn_attestation/signer.go (1)
GetValidatorSigner(122-124)
⏰ 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). (1)
- GitHub Check: acceptance-test
- Refactored the TestComputeAttestationHash function to eliminate unnecessary sub-tests, enhancing readability and reducing complexity. - Consolidated assertions for hash computation when the raw payload is present and when it is missing, ensuring consistent validation of expected outputs. - Updated error messages in the prepareSigningWork function to provide clearer context on attestation hash mismatches. These changes improve the maintainability and clarity of the tests in the tn_attestation extension, ensuring more reliable validation of attestation hash computations.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
extensions/tn_attestation/processor_test.go (1)
25-48: Verify that the hash covers all canonical fields.The test compares
computeAttestationHashoutput against a hash built from a truncated subset (line 41 viabuildHashMaterialat lines 227-240) that excludesBlockHeight,Result, and length prefixes. A past review comment flagged this exact issue, noting that production should hash the full canonical encoding for integrity.If
computeAttestationHashis meant to hash the complete canonical payload, update line 41 to hashpayload.raw(or the full canonical bytes) instead of the truncated subset. If only a subset should be hashed, document why certain fields are excluded and ensure this matches the specification.Run the following script to verify what
computeAttestationHashactually hashes:#!/bin/bash # Description: Check the implementation of computeAttestationHash to determine which fields are hashed. # Search for the function definition ast-grep --pattern 'func computeAttestationHash($$$) $_ { $$$ }'
🧹 Nitpick comments (2)
extensions/tn_attestation/processor_test.go (2)
227-240: Document why buildHashMaterial omits height and result.
buildHashMaterialconstructs a payload subset that excludesBlockHeightandResult. If this is intentional (e.g., for backward compatibility or a specific hashing spec), add a comment explaining why these fields are omitted. Otherwise, consider whether the hash should cover the full canonical encoding for integrity.
288-307: Reduce coupling to SQL implementation details.Lines 297-300 check for
"GROUP BY attestation_hash"in the SQL statement string as a fallback. This couples the test stub to the exact query format, making it fragile if the implementation changes the query structure (e.g., formatting, aliases, subqueries).Consider either:
- Using distinct method signatures for different query types, or
- Configuring the stub with expected query types via a field rather than parsing SQL strings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
extensions/tn_attestation/processor.go(1 hunks)extensions/tn_attestation/processor_test.go(1 hunks)internal/migrations/024-attestation-actions.sql(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- extensions/tn_attestation/processor.go
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-10T13:00:14.189Z
Learnt from: outerlook
PR: trufnetwork/node#1207
File: internal/migrations/024-attestation-actions.sql:58-76
Timestamp: 2025-10-10T13:00:14.189Z
Learning: In the attestation system for internal/migrations/024-attestation-actions.sql, the attestation_hash is computed from (version|algo|data_provider|stream_id|action_id|args) and intentionally excludes created_height. This design ensures the hash is deterministic based only on user input, not network state like block height.
Applied to files:
internal/migrations/024-attestation-actions.sql
📚 Learning: 2025-10-10T13:00:13.731Z
Learnt from: outerlook
PR: trufnetwork/node#1207
File: internal/migrations/023-attestation-schema.sql:20-21
Timestamp: 2025-10-10T13:00:13.731Z
Learning: In the attestations table (internal/migrations/023-attestation-schema.sql), the primary key is (requester, created_height, attestation_hash) because attestation_hash is computed deterministically from user input only (version|algo|data_provider|stream_id|action_id|args) and does not include created_height. This allows the same user to request the same attestation at different block heights.
Applied to files:
internal/migrations/024-attestation-actions.sql
🧬 Code graph analysis (1)
extensions/tn_attestation/processor_test.go (2)
extensions/tn_attestation/canonical.go (1)
ParseCanonicalPayload(40-82)extensions/tn_attestation/signer.go (2)
ResetValidatorSignerForTesting(128-131)InitializeValidatorSigner(112-118)
⏰ 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). (1)
- GitHub Check: acceptance-test
🔇 Additional comments (6)
extensions/tn_attestation/processor_test.go (4)
50-103: LGTM!The test correctly verifies the signing workflow: generates a key, initializes the signer, builds canonical payload, sets up stub engine with appropriate row data, and asserts all critical fields of the prepared signature (hash, payload, created height, 65-byte signature).
105-193: LGTM!The test provides comprehensive end-to-end coverage of the signing and broadcasting flow. It properly verifies the transaction structure, namespace, action name, and all four arguments (hash, requester, created height, signature), ensuring the integration between signing and broadcasting works correctly.
242-269: LGTM!The test correctly verifies batch retrieval of pending hashes with configurable batch sizes, including the fallback behavior when batch size is zero (returns all rows).
365-374: LGTM!The
recordingBroadcastertest double is simple and effective for capturing broadcast calls and inspecting transactions in tests.internal/migrations/024-attestation-actions.sql (2)
89-93: Confirm queue input format and idempotency.tn_attestation.queue_for_signing(encode(...,'hex')) passes lowercase hex without 0x. Please confirm the precompile expects this format and that re‑queuing the same hash is idempotent or safely de‑duplicated under drop‑on‑full semantics.
103-114: Leader gate: verify env semantics.The check @leader_sender == @signer is strict. Confirm this holds with your broadcaster/relayer path; if a relayer signs the tx on behalf of the leader, this would reject. If intended, LGTM.
- Modified the sign_attestation action to include a condition that checks for a null signature, enhancing data integrity during attestation processing. - This change ensures that only valid attestations with a signature are processed, improving the reliability of the attestation workflow in the tn_attestation extension.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/migrations/024-attestation-actions.sql (1)
148-157: Handle rare race between pre-check and UPDATE (concurrent sign).You guard with
AND signature IS NULL(good), but if another leader signs between the SELECT and UPDATE, this UPDATE affects 0 rows and the action still “succeeds.” Add a post-UPDATE verification (or row-count check) to fail fast on races.Apply this follow-up after the UPDATE:
UPDATE attestations SET signature = $signature, validator_pubkey = @signer, signed_height = @height WHERE attestation_hash = $attestation_hash AND requester = $requester AND created_height = $created_height AND signature IS NULL; + +-- Verify the row was actually updated (detect concurrent sign) +$ok BOOL := FALSE; +FOR $r IN + SELECT signature FROM attestations + WHERE attestation_hash = $attestation_hash + AND requester = $requester + AND created_height = $created_height + LIMIT 1 +{ + IF $r.signature IS NOT NULL { $ok := TRUE; } +} +IF NOT $ok { + ERROR('Concurrent sign detected; attestation already signed at height ' || @height::TEXT); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/migrations/024-attestation-actions.sql(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-10T13:00:14.189Z
Learnt from: outerlook
PR: trufnetwork/node#1207
File: internal/migrations/024-attestation-actions.sql:58-76
Timestamp: 2025-10-10T13:00:14.189Z
Learning: In the attestation system for internal/migrations/024-attestation-actions.sql, the attestation_hash is computed from (version|algo|data_provider|stream_id|action_id|args) and intentionally excludes created_height. This design ensures the hash is deterministic based only on user input, not network state like block height.
Applied to files:
internal/migrations/024-attestation-actions.sql
⏰ 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). (1)
- GitHub Check: acceptance-test
🔇 Additional comments (2)
internal/migrations/024-attestation-actions.sql (2)
68-79: Attestation hash excludes height/result — design-aligned and matches Go helpers.
Existing tests in tn_attestation/processor_test.go verify that buildHashMaterial and computeAttestationHash use the same fields and byte order.
89-93: queue_for_signing expects hex‐encoded string
The precompile’sattestation_hashparameter is declared as text, so usingencode($attestation_hash, 'hex')is correct.
2 participants
Description
Complete leader-only attestation signing: canonical payload parsing → secp256k1 signing → broadcast → async status monitoring.
Signing Pipeline
SignDigest()for direct 32-byte hash signing (EVM-compatible)Async Monitoring
sync.OnceSQL Migration
sign_attestation()action enforces leader-only access, stores signatures with validator pubkey + heightRelated Problem
How Has This Been Tested?
Summary by CodeRabbit
New Features
Documentation
Tests
Chores