Skip to content

fix: DeliverMessageSend discards FSM read errors and issues conflicting Set+Delete for drained self-transfers - #1

Open
amathxbt wants to merge 1 commit into
canopy-network:mainfrom
amathxbt:fix/deliver-send-swallowed-error
Open

fix: DeliverMessageSend discards FSM read errors and issues conflicting Set+Delete for drained self-transfers#1
amathxbt wants to merge 1 commit into
canopy-network:mainfrom
amathxbt:fix/deliver-send-swallowed-error

Conversation

@amathxbt

@amathxbt amathxbt commented Jul 4, 2026

Copy link
Copy Markdown

Bug 1 — swallowed FSM error

response, err := c.plugin.StateRead(c, &PluginStateReadRequest{...})
if err != nil {
    return &PluginDeliverResponse{Error: err}
}
// ensure no error fsm error
if response.Error != nil {
    return &PluginDeliverResponse{Error: err} // <-- err is nil here
}

err was already confirmed nil a few lines above, so when response.Error != nil this branch always returns Error: nil — the real FSM-level error is discarded. Execution then falls through to Unmarshal(fromBytes, from) etc. with empty byte slices (since response.Results is empty when response.Error is set), silently treating a failed read as an empty/zero-value account rather than surfacing what actually went wrong.

CheckTx handles the identical situation correctly a few lines above in the same file:

if err == nil {
    err = resp.Error
}

DeliverMessageSend should do the same. Fixed to return response.Error.

Bug 2 — conflicting Set+Delete on the same key

if from.Amount == 0 {
    resp, err = c.plugin.StateWrite(c, &PluginStateWriteRequest{
        Sets: []*PluginSetOp{
            {Key: feePoolKey, Value: feePoolBytes},
            {Key: toKey, Value: toBytes},
        },
        Deletes: []*PluginDeleteOp{{Key: fromKey}},
    })
}

For a self-transfer (msg.FromAddress == msg.ToAddress) that drains the balance to exactly zero, fromKey == toKey (the code even special-cases this above with to = from). This write batch therefore contains both a Set and a Delete for the identical key, which is apply-order-dependent in the underlying store. On a blockchain this is a determinism risk: if different store implementations (or even the same implementation under different internal orderings) resolve Set-vs-Delete-on-same-key differently, nodes could diverge on whether the account ends up deleted or persisted with a zero balance.

Fixed to only include the to Set when it targets an account distinct from from, so a drained self-transfer results in a single unambiguous Delete.

Testing

Traced both code paths by hand against the existing (correct) CheckTx pattern and the self-transfer branch above it. No existing test suite in this repo to extend.

… ops in DeliverMessageSend

Two issues in DeliverMessageSend():

1. When the StateRead response carries an FSM-level error (response.Error
   != nil), the handler returned Error: err instead of Error: response.Error.
   Since err was already confirmed nil a few lines above, this silently
   discarded the real error and let the function fall through to unmarshal
   zero-value account bytes as if the read had simply returned empty state.
   CheckTx already handles this correctly (err = resp.Error); DeliverMessageSend
   did not. Fixed to propagate response.Error.

2. For a self-transfer that drains the balance to exactly zero, fromKey ==
   toKey, but the write batch included both a Set for toKey and a Delete for
   fromKey -- i.e. a Set and a Delete for the identical key in one batch.
   That is undefined/apply-order-dependent in the underlying store, which is
   a state-determinism risk across nodes. Fixed to only include the 'to' Set
   when it targets a distinct account from 'from'.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant