Skip to content

fix: hb_maps:get arity misuse in genesis/delegated checkpoint flow + checkpoint data presence checks#702

Open
charmful0x wants to merge 4 commits intoedgefrom
audit/delegated-compute
Open

fix: hb_maps:get arity misuse in genesis/delegated checkpoint flow + checkpoint data presence checks#702
charmful0x wants to merge 4 commits intoedgefrom
audit/delegated-compute

Conversation

@charmful0x
Copy link

about

following the work done on #701 i did more hb_maps:get/3 usage checks looking for default misuse.

dev_delegated_compute

hb_maps:get(<<"type">>, Snapshot, Opts) == <<"Checkpoint">> in dev_delegated_compute was calling get with the incorrect arity (passing Opts as default). fixed that to use get 4 arity. the same for Body = hb_maps:get(<<"data">>, Snapshot, Opts),

additionally for the Body patch, i added fail fast throw when the checkpoint data body isnt available

dev_genesis_wasm

the same incorrect hb_maps:get arity was used at CheckpointTargetProcID = hb_maps:get(<<"process">>, CheckpointMessage, Opts) - patched that.

additionally the import_legacy_checkpoint/0 test does:

    ?assertMatch(
        #{ <<"data">> := Data } when byte_size(Data) > 0,
        hb_maps:get(<<"snapshot">>, ProcWithCheckpoint)
    ),

but do_import/3 already validates signer/process/nonce, but it didnt validate checkpoint payload presence. so i added a data presence check at ingress (not_found rejection):

        CheckpointData = hb_maps:get(<<"data">>, CheckpointMessage, not_found, Opts),
        true ?= CheckpointData =/= not_found orelse missing_checkpoint_data,

clarity required

~genesis-wasm@1.0 restore flow goes through ~delegated-compute@1.0 (dev_genesis_wasm:normalize/3 -> dev_delegated_compute:normalize/3 -> load_state/2), so checkpoint payload assumptions should stay aligned both at ingress (do_import/3) and at restore execution (load_state/2)

should the checkpoint point integrity policy be stricter in both dev_genesis_wasm and dev_delegated_compute to restrict 'existing but empty checkpoint' data payloads (following the import_legacy_checkpoint test)?

@charmful0x
Copy link
Author

also i added a fix for an hb_maps:get/3 opts default misuse in dev_process restore path (ensure_loaded/3): Opts was being passed as the default for <<"process">>

if process was missing, this could propagate invalid state and fail later in commitment merge/update with silent errors. now it fail fast with typed throw and validate Process as map

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