Track fee receivers on chain and validate them at the indexed tip - #101
Track fee receivers on chain and validate them at the indexed tip#101janezpodhostnik wants to merge 2 commits into
Conversation
Previously validateFeeReceivers gave up for good after 5 quick attempts, so a flaky access node at startup plus a stale config could leave the server running with an unvalidated fee set indefinitely, and the only signal was a single log line. - Retry forever: quick backoff for the first 5 attempts, then a slow one-minute poll. Treat malformed script results as retryable instead of giving up. - Re-check every 10 minutes after a definitive result, so receivers added on chain while the server is running are also detected. - Downgrade a config mismatch from a fatal exit to a logged error that is surfaced via the new fee_receiver_validation_status /call method, matching the balance_validation_status pattern. - Replace the stringly-typed validation status with a validationStatus enum shared by balance and fee validation. - Add tests for the fee validation state machine, including a concurrent access test for the race detector.
📝 WalkthroughWalkthroughFee receiver changes are now indexed by block, persisted by height, and used for fee classification. Background validation retries and rechecks receiver data. Typed validation status, receiver details, and errors are exposed through ChangesFee receiver tracking and persistence
Fee receiver validation and reporting
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant BlockProcessor
participant Store
participant Validator as validateFeeReceivers
participant AccessNode
participant CallService as /call
participant Client
BlockProcessor->>Store: persist fee receiver changes by height
Validator->>Store: read latest indexed receiver set
Validator->>AccessNode: validate fee receivers
AccessNode-->>Validator: receiver data or validation error
Validator->>CallService: store typed validation status
Client->>CallService: request fee_receiver_validation_status
CallService-->>Client: status and receiver details
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
api/call_service.go (1)
215-230: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider reporting the time of the last definitive check.
setFeeValidationRetryingpreserves a terminal result, so repeated access node failures do not change the reported status. An operator cannot then distinguish a freshsuccessfrom one recorded hours ago while every recheck since has failed. Store the timestamp of the last definitive result and the last attempt, then include them in this response.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/call_service.go` around lines 215 - 230, Update the fee validation status flow around Server.feeReceiverValidationStatus and setFeeValidationRetrying to track both the timestamp of the last definitive validation result and the timestamp of the most recent check attempt, preserving terminal results across access-node failures. Include both timestamps in the returned result map so callers can distinguish a fresh status from a stale preserved result.api/api.go (2)
373-388: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueAn unknown
validationStatusterminates the process from the/callrequest path. Both default branches calllog.Fatalf, which callsprocess.Exit(1)(log/log.go:78-81). The branches are unreachable for the current four enum values. If a fifth status is added later without updating both switches, one API request stops the server. Handle the unknown value without exiting.
api/api.go#L373-L388: return a fallback string such asfmt.Sprintf("unknown(%d)", int(v))instead of callinglog.Fatalfand panicking.api/call_service.go#L209-L211: replace the fatal default withlog.Errorfplus a response that carriesv.status.String(), so the handler degrades instead of exiting.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/api.go` around lines 373 - 388, The unknown validationStatus handling must degrade without terminating the server. In api/api.go at lines 373-388, update validationStatus.String to return a formatted fallback containing the numeric value instead of calling log.Fatalf or panicking; in api/call_service.go at lines 209-211, replace the fatal default with log.Errorf and return a response carrying v.status.String(), preserving normal handling for known statuses.
255-270: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLog the mismatch only on a status transition.
checkFeeReceiversre-runs everyfeeValidateRecheckInterval, so an unresolved mismatch logs the identical error every 10 minutes for the lifetime of the process.setFeeValidationSuccessalready logs only on transitions. Apply the same rule here to keep the log volume symmetric. The/callmethod still reports the persisted failure.♻️ Proposed change to log only on transitions
- log.Errorf("%s", msg) s.feeValidationMu.Lock() - defer s.feeValidationMu.Unlock() + prev := s.feeValidation.status s.feeValidation = &feeValidation{ err: msg, missing: missing, onchain: onchain, status: validationFailure, } + s.feeValidationMu.Unlock() + // We only log on transitions so that the periodic re-checks don't flood + // the logs. + if prev != validationFailure { + log.Errorf("%s", msg) + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/api.go` around lines 255 - 270, Update setFeeValidationFailure to log the mismatch only when the existing fee validation status transitions into validationFailure, while continuing to persist the latest failure details on every check. Match the transition-aware behavior of setFeeValidationSuccess and preserve /call reporting through the stored feeValidation state.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@api/api.go`:
- Around line 373-388: The unknown validationStatus handling must degrade
without terminating the server. In api/api.go at lines 373-388, update
validationStatus.String to return a formatted fallback containing the numeric
value instead of calling log.Fatalf or panicking; in api/call_service.go at
lines 209-211, replace the fatal default with log.Errorf and return a response
carrying v.status.String(), preserving normal handling for known statuses.
- Around line 255-270: Update setFeeValidationFailure to log the mismatch only
when the existing fee validation status transitions into validationFailure,
while continuing to persist the latest failure details on every check. Match the
transition-aware behavior of setFeeValidationSuccess and preserve /call
reporting through the stored feeValidation state.
In `@api/call_service.go`:
- Around line 215-230: Update the fee validation status flow around
Server.feeReceiverValidationStatus and setFeeValidationRetrying to track both
the timestamp of the last definitive validation result and the timestamp of the
most recent check attempt, preserving terminal results across access-node
failures. Include both timestamps in the returned result map so callers can
distinguish a fresh status from a stale preserved result.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8bbec699-e874-4e94-88ec-c2804a4266d8
📒 Files selected for processing (5)
README.mdapi/api.goapi/call_service.goapi/validate.goapi/validate_test.go
tim-barry
left a comment
tim-barry
left a comment
There was a problem hiding this comment.
Overall it makes sense, and I do think the polling/retry functionality is good at startup, but I have a few additional concerns/suggestions:
-
How does fee receiver validation interact with a chain/historical spork where FlowFees hasn't been upgraded to onflow/flow-core-contracts#575 ? Ideally we should ensure that it (relatively silently) falls back to using only the main FlowFees address, but we do still need to keep checking in case FlowFees is upgraded and new fee receivers are added in the future of the chain.
-
I would prefer if we directly checked for the
ChildFeeAccountsChangedevent while indexing and used that to update our set of fee receiver addresses; storing these events in the index database (with a new prefix) and then allowing the most recent of those to override the initial/default configured fee address(es) for a chain seems to me to be a more robust solution than logging an error and requesting a human to update the config, as the validation will currently do when the set of fee receivers changes.
I believe it would also fit better with Rosetta's model if, at startup, we queried the fee receiver addresses at the latest indexed block (or genesis block if no index exists), rather than the current latest block available on the Access API.
janezpodhostnik
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@state/process.go`:
- Around line 662-709: Preserve event ordering in the event-processing flow
around the i.typFeeAcctsChanged handler and evtloop2: record each fee receiver
set change by event position instead of immediately replacing feeAddrs for the
whole transaction. While classifying FlowToken.TokensDeposited events, apply
only the latest receiver change that precedes that deposit, so earlier deposits
use the prior set; add coverage for deposits before and after a receiver-change
event.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5ae5d722-5050-450d-ae3f-e6c1474e0e6b
📒 Files selected for processing (11)
README.mdapi/api.goapi/construction_service.goapi/validate.goapi/validate_test.goconfig/config.goconfig/config_test.goindexdb/indexdb.goindexdb/indexdb_test.gostate/process.gostate/state.go
🚧 Files skipped from review as they are similar to previous changes (4)
- README.md
- api/validate.go
- api/validate_test.go
- api/api.go
| case i.typFeeAcctsChanged: | ||
| // NOTE: The event carries the complete list of child fee | ||
| // accounts. We update our set of fee addresses here, in the | ||
| // first event loop, so that the fee deposits of this and all | ||
| // subsequent transactions are classified with the updated set | ||
| // — fee deduction runs after the transaction body that emits | ||
| // the event. | ||
| event, err := decodeEvent("FlowFees.ChildFeeAccountsChanged", evt, hash, height) | ||
| if err != nil { | ||
| skipCache = true | ||
| continue outer | ||
| } | ||
| fields := event.FieldsMappedByName() | ||
| if len(fields) != 1 { | ||
| log.Errorf( | ||
| "Found FlowFees.ChildFeeAccountsChanged event with %d fields in transaction %x in block %x at height %d", | ||
| len(fields), txnHash, hash, height, | ||
| ) | ||
| skipCache = true | ||
| continue outer | ||
| } | ||
| // 'addresses' field | ||
| arr, ok := cadence.SearchFieldByName( | ||
| event, | ||
| "addresses", | ||
| ).(cadence.Array) | ||
| if !ok { | ||
| log.Errorf( | ||
| "Unable to load addresses from FlowFees.ChildFeeAccountsChanged event in transaction %x in block %x at height %d", | ||
| txnHash, hash, height, | ||
| ) | ||
| skipCache = true | ||
| continue outer | ||
| } | ||
| feeReceivers = [][]byte{} | ||
| for _, val := range arr.Values { | ||
| addr, ok := val.(cadence.Address) | ||
| if !ok { | ||
| log.Errorf( | ||
| "Unable to convert FlowFees.ChildFeeAccountsChanged element to an address (got %T) in transaction %x in block %x at height %d", | ||
| val, txnHash, hash, height, | ||
| ) | ||
| skipCache = true | ||
| continue outer | ||
| } | ||
| feeReceivers = append(feeReceivers, addr[:]) | ||
| } | ||
| feeAddrs = i.Chain.Contracts.FeeAddressesWith(feeReceivers) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Preserve event order when applying fee receiver changes.
Line 709 changes feeAddrs in the first event loop. evtloop2 then classifies every FlowToken.TokensDeposited event in the transaction with that new set. A deposit emitted before FlowFees.ChildFeeAccountsChanged can therefore become a fee deposit incorrectly.
Track receiver changes by event position and apply each change only to later deposits. Add a test with deposits both before and after a receiver-change event.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@state/process.go` around lines 662 - 709, Preserve event ordering in the
event-processing flow around the i.typFeeAcctsChanged handler and evtloop2:
record each fee receiver set change by event position instead of immediately
replacing feeAddrs for the whole transaction. While classifying
FlowToken.TokensDeposited events, apply only the latest receiver change that
precedes that deposit, so earlier deposits use the prior set; add coverage for
deposits before and after a receiver-change event.
|
@tim-barry Thanks for the review! I dont have a lot of experience with how this repo is used, but I think I got your point (please let me know if I'm still off). I made the code resilient to the current state the fee accounts are in, and I made it read the events to update the child fee accounts on the fly. |
2 participants
Review feedback on the validation-only approach: rather than just checking the configured fee addresses, learn receiver changes from the chain and align validation with what the indexer is actually classifying.
Changes
FlowFees.ChildFeeAccountsChangedevents and store them per height in the index database under a newfprefix. The most recent event at or before a block overrides the configured fee addresses when classifying that block's deposits, so receivers added on chain are picked up without a config update or restart. The configured addresses remain the base, which matters for testnet: its child accounts were registered by a direct storage write that emitted no event (tx be210889, height 309507846).getFeeReceiverAddresses(pre-upgrade FlowFees) as a definitive result: the FlowFees account is the only receiver. Logged once, and polling continues so a later upgrade is detected.fee_receiver_validation_status/callmethod.resync_fromremedy for testnet index databases that hold blocks indexed beforefee_receiversexisted.Related: #100, onflow/flow-core-contracts#575
Summary by CodeRabbit
New Features
/callinterface.Bug Fixes
Documentation