chore: force deterministic args for attestation queries#1215
Conversation
Add tn_utils.force_last_arg_false precompile to override use_cache parameter in attestation requests, ensuring all validators compute identical results regardless of cache state. Changes: - Add force_last_arg_false() precompile to tn_utils - Update request_attestation to force use_cache=false for query actions - Add comprehensive tests covering edge cases Fixes non-deterministic attestation results when users pass use_cache=true for composed stream queries. Query actions (IDs 1-5) now automatically override the last parameter to false before executing via call_dispatch. resolves: #1208
Time Submission Status
|
WalkthroughAdds a tn_utils precompile method force_last_arg_false with a handler that forces the last encoded action argument to false. Integrates it into the precompile registry. Updates attestation SQL to call this method for actions 1–5 before dispatch. Introduces comprehensive tests for the handler. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant SQL as SQL (attestation)
participant TN as tn_utils.precompile
participant Disp as call_dispatch
Client->>SQL: request_attestation(action_id, args_bytes)
alt action_id in [1..5]
SQL->>TN: force_last_arg_false(args_bytes)
TN->>TN: decode args, set last arg=false, encode
TN-->>SQL: modified_args_bytes
SQL->>Disp: call_dispatch(action_id, modified_args_bytes)
else other actions
SQL->>Disp: call_dispatch(action_id, args_bytes)
end
Disp-->>Client: result/status
note over TN: New mutation step ensures last arg is false when applicable
sequenceDiagram
autonumber
participant Ext as tn_utils.precompile
participant Handler as forceLastArgFalseHandler
Ext->>Handler: handle(args_bytes)
alt decodable args
Handler->>Handler: parse list
alt empty list
Handler-->>Ext: return original args_bytes
else non-empty
Handler->>Handler: set last arg=false
Handler-->>Ext: return modified_args_bytes
end
else decode error
Handler-->>Ext: error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![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: 0
🧹 Nitpick comments (1)
extensions/tn_utils/serialization_test.go (1)
465-577: Excellent test coverage with one enhancement opportunity.The
TestForceLastArgFalseHandlertest suite provides comprehensive coverage:✅ Covered scenarios:
- Normal case: forcing
truetofalsewith multiple arguments- Idempotency: verifying already-false values remain false
- Edge cases: empty args and single argument
- Error handling: invalid bytes and wrong input type
The tests correctly validate the transformation by encoding, processing through the handler, and decoding to verify the last argument is
false.Enhancement opportunity: Add a test case for type safety when the last argument is not boolean:
t.Run("non-boolean last arg returns error on re-encode", func(t *testing.T) { // Args with a string as the last parameter instead of boolean original := []interface{}{ "provider", "stream", int64(100), int64(200), nil, "not_a_boolean", // Last arg is string, not boolean } argsBytes, err := EncodeActionArgs(original) require.NoError(t, err) // Attempting to force last arg to false should fail during re-encoding // because we're replacing a string with a boolean err = forceLastArgFalseHandler(nil, nil, []any{argsBytes}, func([]any) error { return nil }) // Depending on implementation, this might succeed (if types.EncodeValue accepts bool) // or fail (if there's type checking). Document the expected behavior. // If the implementation allows this, verify the result is a boolean false // If not, expect an error })This would document and test the behavior when the type assumptions are violated.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
extensions/tn_utils/precompiles.go(2 hunks)extensions/tn_utils/serialization_test.go(1 hunks)internal/migrations/024-attestation-actions.sql(1 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
🧬 Code graph analysis (2)
extensions/tn_utils/serialization_test.go (1)
extensions/tn_utils/serialization.go (2)
EncodeActionArgs(45-76)DecodeActionArgs(85-130)
extensions/tn_utils/precompiles.go (1)
extensions/tn_utils/serialization.go (2)
DecodeActionArgs(85-130)EncodeActionArgs(45-76)
⏰ 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 (5)
extensions/tn_utils/precompiles.go (4)
28-28: LGTM! Precompile method correctly registered.The
forceLastArgFalseMethod()is properly added to theMethodslist in the precompile bundle, ensuring it's accessible from SQL migrations via thetn_utilsnamespace.
347-364: LGTM! Method definition follows established patterns.The
forceLastArgFalseMethodis well-structured with:
- Appropriate access modifiers (VIEW, PUBLIC)
- Clear parameter and return type definitions
- Consistent naming that conveys intent
366-394: Verify type safety when the last argument is not boolean.The handler correctly decodes, modifies, and re-encodes the arguments. However, there's no type checking before forcing the last argument to
false(line 385). If the last argument is not boolean-compatible, the subsequent re-encoding might fail or produce unexpected results.While the current use case (forcing
use_cacheparameter) guarantees the last argument is boolean, consider adding defensive type checking for robustness:// Force last parameter to false (for use_cache) +// Verify the last argument is boolean before forcing +if _, ok := args[len(args)-1].(bool); !ok { + // Check for boolean pointer as well + if ptr, ok := args[len(args)-1].(*bool); !ok || ptr == nil { + return fmt.Errorf("last argument must be boolean, got %T", args[len(args)-1]) + } +} args[len(args)-1] = falseAlternatively, document that this handler is specifically designed for actions where the last parameter is guaranteed to be boolean, and add a test case to verify the error behavior when the last argument has an incompatible type.
380-382: Edge case handling is correct.Returning the original
argsBytesunchanged when there are no arguments is the appropriate behavior. This prevents unnecessary processing and avoids errors from attempting to modify a non-existent last argument.internal/migrations/024-attestation-actions.sql (1)
40-46: Verify parameter ordering for query actions (1–5)
Ensureuse_cacheis the final parameter for all actions with IDs 1–5 (get_record, get_index, get_change_over_time, get_last_record, get_first_record); mismatches will break the deterministic enforcement.
2 participants
Add tn_utils.force_last_arg_false precompile to override use_cache parameter in attestation requests, ensuring all validators compute identical results regardless of cache state.
Changes:
Fixes non-deterministic attestation results when users pass use_cache=true for composed stream queries. Query actions (IDs 1-5) now automatically override the last parameter to false before executing via call_dispatch.
resolves: #1208
Summary by CodeRabbit
Bug Fixes
Tests