Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
node: Flow cancel enhancements and bug fixes (#4016)
* node: Fix issue where transfers that were loaded from the DB did not add a flow-cancel transfer on the TargetChain Flow-canceling is done in the `ProcessMsgForTime` loop when a new message occurs. However, this was not done when a node restarted and reloaded transfers from the past 24 hours. As a result it was possible for the node to calculate a result that showed that the outgoing transfers for an emitter chain exceeded the daily limit. In effect this is true but only with the condition that there was incoming flow to allow this to happen. This appeared to violate an invariant and so the node did not start properly. node: Add unit tests when reloading flow cancel transactions from the database node: fix lint errors in governor_test.go * node: Add a command-line flag to enable or disable flow-canceling on restart Added a command-line flag to enable or disable flow-canceling when starting the node. This should allow Guardians to disable flow canceling in the case of future bugs or during a security incident. This should prevent the need to rollback to earlier Guardian versions. (@mdulin2 ) * node: Use deterministic iteration order over chains when changing Governor state - Adds a field that stores a sorted slice of chain IDs to the governor. - Use this field to iterate in a determinstic order when performing actions that change the state of the Governor - This should help Guardians reach a more similar view of the Governor in scenarios where iteration order might impact whether a transfer is queued. (This is relevant especially in the case of Flow Canceling) - Cases where only a single VAA is being modified were not changed. Iteration order should not matter here and determinstic order may may worse for performance when searching for a particular element. * node: Fix tokenEntry when checking flow cancel for pending transfers (Squash and merge bug fix from PR #4001) Similar to a previous issue in the function `ProcessMsgForTime`, the tokenEntry was not being generated properly. This should result in queued "small transfers" being able to flow cancel when they are released from the queue. Also adds a comment on the CheckedInt64 function to indicate what its error states mean and when they occur. Add comments and change variable names for governor_monitoring - Add function comments to explain what they do and what their error states mean - Adds governor logging to error cases - Change variable names in publishStatus function. `value` was used first to indicate the "governor usage" and then reused to indicate the remaining available notional value for a chain. This refactor tries to make it clear that these are different concepts Add unit test for flow cancelling when a pending transfer is released - Add a unit test to ensure that, when a pending transfer is released, it also does flow-cancelling on the TargetChain (previously we had a bug here) - Add documentation for CheckPendingForTime to clarify that it has side-effects * node: Modify error handling for CheckPending method in the Governor Previous rollouts of the Flow Cancel feature contained issues when calculating the Governor usage when usage was near the daily limit. This caused an invariant to be violated. However, this was propagated to the processor code and resulted in the processor restarting the entire process. Instead, the Governor should simply fail-closed and report that there is no remaining capacity, causing further VAAs to be queued until the usage diminishes over time. The circumstances leading to the invariant violations are not addressed in this commit. Instead this commit reworks the way errors are handled by the CheckPending, making careful choices about when the process should or should not be killed. - Change "invariant" error handling: instead of causing the process to die, log an error and skip further for a single chain while allowing processing for other chains to continue - Remove 'invariant error' in TrimAndSumValueForChain as it can occur somewhat regularly with the addition of the flow cancel feature - Return dailyLimit in error condition rather than 0 so that future transfers will be queued - Do not cap the sum returned from TrimAndSumValueForChain: instead allow it to exceed the daily limit. - Modify unit tests to reflect this - Add unit tests for overflow/underflow scenarios in the TrimAndSumValue functions - Change other less severe error cases to log warnings instead of returning errors. - Generally prevent flow-cancel related issues from affecting normal Governor operations. Instead the flow cancel transfers should simply not be populated and thus result in "GovernorV1" behavior. - Add documentation to CheckPendingForTime to explain the dangers of returning an error - Reword error messages to be more precise and include more relevant fields. Add documentation explaining when the process should and should not die * node: Add additional metrics for Governor status Modify the monitoring code and protobuf files to make the status of the Governor more legible when flow-canceling is enabled. This can be consumed by Wormhole Dashboard to better reflect the effects of flow cancelling. On the level of the Governor: - whether the Guardian has enabled flow cancel or not On the level of the Governor's emitters, reports 24h metrics for: - net value that has moved across the chain - total outgoing amount - total incoming flow cancel amount Currently big transfers are not accounted for as they do not affect the Governor's capacity. (They are always queued.) * node: Add new flow cancel parameter to Governor in tests * node: goimports formatting * node: Bug fix in changes to governor monitoring - Fix issue where stats weren't being populated unless flow cancel was enabled - Fix wrong return value used in unit test - Fix typo in proto variable name - Move sorting outside of a for loop for efficiency - Restore unit test that was deleted in the process of rebasing * node: address prealloc lint error in governor code * node: Fix "generated proto differs from committed proto" * node: Fix bug in chainIds allocation - This resolves a mistake with allocating the chainIds in the governor initialization that causes nil entries in the slice. - Add unit tests to ensure that the chainIds slice matches the chains map - Add unit test to ensure that TrimAndSumValueForChain checks for a nil pointer to avoid panics * node: Fix returning nil on err in governor_test.go * node: Cleanup comments in governor code * node: fix governor comment * node: enable flow cancel in governor_monitoring tests * node: Add flow cancel information to p2p heartbeat features * node: Remove outdated comment from governor * node: Upgrade logs to Error from Warn when reloading transfers from database * node: Enable flow cancel in check_query test function * node: Cleanup comments and redundant code in governor * node: Refactor how the flow cancel token list gets populated - Only populate the flow cancel tokens list once - Change default behavior to use an empty flow cancel assets list, rather than first populating the list and then clearing it - Refactor the logic around enabling the flow cancel token field for governed assets. Now it only executes if flow cancel is enabled, rather than operating over an empty slice when flow cancel is disabled - Modify devnet/testnet configs so that they are responsible for returning the correct list of flow cancelling assets * node: Add unit test for flow cancel feature flag * node: Move new Governor status proto fields from Emitter to Chain * node: lint governor_monitoring --------- Co-authored-by: Maxwell Dulin <[email protected]>
- Loading branch information
1 parent
038d76b
commit 5042ff1