lib/recovery + unrollplan: recovery proof graph and pure planner (2/5) - #261
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a recovery proof system and a planner for unilateral-exit execution. It includes logic for building transaction graphs, tracking confirmation status, and calculating CSV maturity. Feedback focuses on improving efficiency by using byte comparisons for hash sorting and avoiding redundant map lookups for layer indices. Additionally, a validation check is suggested to ensure consistency between target confirmation status and the presence of a confirmation height.
| func sortHashes(hashes []chainhash.Hash) { | ||
| sort.Slice(hashes, func(i, j int) bool { | ||
| return hashes[i].String() < hashes[j].String() | ||
| }) | ||
| } |
There was a problem hiding this comment.
Sorting hashes by their string representation is inefficient as it requires hex encoding each hash twice per comparison. Using bytes.Compare on the raw bytes is significantly faster and achieves the same deterministic result.
| func sortHashes(hashes []chainhash.Hash) { | |
| sort.Slice(hashes, func(i, j int) bool { | |
| return hashes[i].String() < hashes[j].String() | |
| }) | |
| } | |
| func sortHashes(hashes []chainhash.Hash) { | |
| sort.Slice(hashes, func(i, j int) bool { | |
| return bytes.Compare(hashes[i][:], hashes[j][:]) < 0 | |
| }) | |
| } |
| for _, layer := range s.proof.layers { | ||
| for _, txid := range layer { | ||
| state := s.txStates[txid] | ||
|
|
||
| switch state { | ||
| case TxStateBroadcasted: | ||
| awaiting = append(awaiting, txid) | ||
|
|
||
| case TxStateConfirmed: | ||
| continue | ||
|
|
||
| case TxStatePending: | ||
| missingParents, err := s.missingParents(txid) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| layerIndex, err := s.proof.Layer(txid) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
The topological layer index is already available from the outer loop iteration. Calling s.proof.Layer(txid) inside the loop performs an unnecessary map lookup for every pending transaction. Using the loop index directly is more efficient.
| for _, layer := range s.proof.layers { | |
| for _, txid := range layer { | |
| state := s.txStates[txid] | |
| switch state { | |
| case TxStateBroadcasted: | |
| awaiting = append(awaiting, txid) | |
| case TxStateConfirmed: | |
| continue | |
| case TxStatePending: | |
| missingParents, err := s.missingParents(txid) | |
| if err != nil { | |
| return nil, err | |
| } | |
| layerIndex, err := s.proof.Layer(txid) | |
| if err != nil { | |
| return nil, err | |
| } | |
| for i, layer := range s.proof.layers { | |
| for _, txid := range layer { | |
| state := s.txStates[txid] | |
| switch state { | |
| case TxStateBroadcasted: | |
| awaiting = append(awaiting, txid) | |
| case TxStateConfirmed: | |
| continue | |
| case TxStatePending: | |
| missingParents, err := s.missingParents(txid) | |
| if err != nil { | |
| return nil, err | |
| } | |
| layerIndex := i |
| if s.TargetConfirmHeight != nil && !confirmedTxidSetContains( | ||
| confirmed, proof.TargetOutpoint().Hash, | ||
| ) { | ||
|
|
||
| return fmt.Errorf("target confirm height set " + | ||
| "without confirmed target") | ||
| } |
There was a problem hiding this comment.
The validation logic should enforce that if the target transaction is confirmed, a TargetConfirmHeight must be provided. This ensures consistency and prevents potential issues in the Plan method when calculating CSV maturity, which currently returns an error if the height is missing for a confirmed target.
| if s.TargetConfirmHeight != nil && !confirmedTxidSetContains( | |
| confirmed, proof.TargetOutpoint().Hash, | |
| ) { | |
| return fmt.Errorf("target confirm height set " + | |
| "without confirmed target") | |
| } | |
| targetConfirmed := confirmedTxidSetContains( | |
| confirmed, proof.TargetOutpoint().Hash, | |
| ) | |
| if s.TargetConfirmHeight != nil && !targetConfirmed { | |
| return fmt.Errorf("target confirm height set " + | |
| "without confirmed target") | |
| } | |
| if targetConfirmed && s.TargetConfirmHeight == nil { | |
| return fmt.Errorf("target confirmed without " + | |
| "target confirm height") | |
| } |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 58c2e1f91e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| txStates := make(map[chainhash.Hash]TxState, len(proof.nodes)) | ||
| for txid := range proof.nodes { |
There was a problem hiding this comment.
NewSession dereferences proof.nodes before any validation, so a nil proof causes an immediate panic and process crash on construction (for example, if proof loading fails but construction is still attempted). This constructor is part of the public recovery API, so it should fail in a controlled way instead of panicking.
Useful? React with 👍 / 👎.
| if js.FailedTxid != nil { | ||
| hash, err := parseHash(*js.FailedTxid) | ||
| if err != nil { | ||
| return fmt.Errorf("failed_txid %q: %w", | ||
| *js.FailedTxid, err) | ||
| } | ||
|
|
||
| s.FailedTxid = &hash |
There was a problem hiding this comment.
UnmarshalJSON only sets s.FailedTxid when failed_txid is present, but never clears it when the field is absent. If a SessionState object is reused across decodes, an old failure txid can leak into a newer state and incorrectly mark the restored session as failed or inconsistent.
Useful? React with 👍 / 👎.
2ce1d2d to
8120a07
Compare
|
@claude review this |
|
Claude Code is working… I'll analyze this and get back to you. |
58c2e1f to
bf7f490
Compare
|
@claude review this |
|
Claude finished @Roasbeef's task in 2m 10s —— View job PR Review: lib/recovery + unrollplan (2/5)Tasks:
Overall AssessmentSolid, well-structured implementation. The separation between the immutable proof graph ( Bugs / Correctness1. The current validation rejects targetConfirmed := confirmedTxidSetContains(
confirmed, proof.TargetOutpoint().Hash,
)
if s.TargetConfirmHeight != nil && !targetConfirmed {
return fmt.Errorf("target confirm height set " +
"without confirmed target")
}
if targetConfirmed && s.TargetConfirmHeight == nil {
return fmt.Errorf("target confirmed without " +
"target confirm height")
}There's also no test for this second case. Fix this → 2.
func NewSession(proof *Proof) (*Session, error) {
if proof == nil {
return nil, fmt.Errorf("proof cannot be nil")
}
// ...
}(This also changes the signature to return 3. If a // current code only sets FailedTxid when present:
if js.FailedTxid != nil {
hash, err := parseHash(*js.FailedTxid)
...
s.FailedTxid = &hash
}
// missing: s.FailedTxid = nil in the else branchIn practice 4.
Performance5. Both copies of func sortHashes(hashes []chainhash.Hash) {
sort.Slice(hashes, func(i, j int) bool {
return bytes.Compare(hashes[i][:], hashes[j][:]) < 0
})
}Same fix applies to the 6. Redundant The outer loop already iterates // Before:
for _, layer := range s.proof.layers {
for _, txid := range layer {
...
layerIndex, err := s.proof.Layer(txid) // redundant
// After:
for i, layer := range s.proof.layers {
for _, txid := range layer {
...
layerIndex := i // already known7.
Minor / Style8. // Current:
session.lastError = fmt.Errorf("%s", state.LastError)
// Cleaner:
session.lastError = errors.New(state.LastError)
9. Blank assignment The variable _, hasConfirmHeight := state.ConfirmHeights[txid]Missing Test Cases
Summary
|
Add the immutable recovery proof graph (Proof, Node, NodeKind) with topological layering and parent/child tracking. Add the recovery Session that tracks per-node broadcast/confirmation state and computes CSV maturity. Add SessionState with custom JSON marshaling for map[chainhash.Hash] keys. Add proof binary codec for checkpoint persistence. Add tree path extraction helpers. Includes comprehensive unit tests for proof construction, session lifecycle, multi-branch graphs, and state round-trip serialization.
Pure computation layer that answers: given a recovery proof, durable progress state, and current block height, which proof transactions are ready to broadcast, which are blocked, and when is the target CSV-mature for sweeping? No I/O, no actors — callers own the durable State and the planner re-derives everything from first principles on each call. Validates state consistency against the proof graph before planning. Key types: Planner, State, Snapshot, TxFrontier, CSVInfo, SweepState.
Replace JSON-based SessionState codec with a TLV codec. The prior JSON path went through chainhash.NewHashFromStr which zero-pads short hex inputs, so distinct JSON keys could collapse to one Go map entry (parser-differential; last-write-wins). Raw 32-byte hashes eliminate that attack surface and match the canonical on-disk form used elsewhere in the repo. Add a Proof binary codec (proof_codec.go) so callers can checkpoint the immutable graph alongside the SessionState; this fulfills the "proof binary codec for checkpoint persistence" promise in the PR description. Switch optional pointer fields (FailedTxid, Snapshot.CSV, Snapshot.FailedTxid) to fn.Option for consistent nil-safety and idiomatic WhenSome / UnwrapOrErr / UnwrapOrFail handling. Both codecs carry a version byte so future wire-format changes can be migrated explicitly rather than silently re-interpreting older blobs. Duplicate keys, wrong-length hashes, unknown versions, and trailing bytes all fail the decode loudly.
bf7f490 to
c0731c8
Compare
Strip json:"..." tags from unrollplan.State and SweepState and add a dedicated TLV codec (state_codec.go) so the planner state gets the same adversarial-input guarantees as the recovery-side codec: raw 32-byte hashes, duplicate-key rejection, version byte, and length-mismatch failures. Move optional fields (TargetConfirmHeight, SweepState.Txid, SweepState.ConfirmHeight, Snapshot.CSV) from nil-able pointers to fn.Option. Callers now use WhenSome / UnwrapOrErr / UnwrapOrFail instead of nil checks, which removes the can-I-deref-this ambiguity and plays cleanly with MapOptionZ-style error forwarding. csvInfoAt and validateSweepState take fn.Option[int32] directly; Plan no longer needs copyInt32Ptr.
Add rapid property-based tests for the SessionState, unrollplan.State, and Proof TLV codecs covering round-trip equivalence and canonical encoding. The tests shrink to minimum counterexamples on failure, giving us adversarial-input coverage without a hand-enumerated fuzz corpus. Add focused unit tests for the state-machine guards landed in the preceding fixup commits (MarkConfirmed parent-confirmed / height negative / re-confirm-at-different-height, MarkFailed overwrite rejection, ComputeMaturityHeight overflow, sweep state CSV maturity and txid-collision checks). Add a concurrency test that hammers Session from multiple goroutines; combined with -race it validates the sync.RWMutex added for H-3. Coverage: lib/recovery 43% -> 85%, unrollplan 47% -> 86%.
Per-package documentation for the two new recovery packages, plus an entry in ARCHITECTURE.md's Layer 1 table. Documents the invariants that are not self-evident from the code: the csvDelay unit + MaxCSVDelay cap, MarkConfirmed's state-machine guards, the sweep ordering rules, and the canonical TLV encoding.
c0731c8 to
9e35a2d
Compare
Expand core files with walkthrough-style comments explaining the
mental model, algorithmic choices, and non-obvious invariants.
lib/recovery:
- Add package-level doc.go with the unilateral-exit data model and
key invariants.
- NodeKind and Node: explain metadata-vs-planning split and the
unsigned/signed-tx invariance.
- Proof: document the redundancy between parents/children/layers/
layerByTxid and which hot-path query each serves.
- NewProof: five-stage validation narrative with rationale for
duplicate rejection, in-graph-only edges, reachability, and the
Kahn-style layering.
- buildLayers: Kahn's algorithm commentary plus cycle detection
side-effect explanation.
- Session: caller-driven state machine with textual transition
diagram; concurrency rules; why internal helpers assume the lock.
- MarkBroadcasted / MarkConfirmed: per-guard explanation of why each
rejection exists (references the C/H findings from the PR review).
- SnapshotAt: layer-walk classification narrative.
- ComputeMaturityHeight: signed-overflow and unsigned-to-signed
overflow reasoning.
- SessionState / validateSessionState: persistence philosophy, the
"mirror every invariant" rule, ordered-checks rationale.
- proof_codec.go: refactor Node encoding to use Node.Record() +
nested TLVs so per-Node fields are forward-extensible.
unrollplan:
- Expand doc.go with the mental model, no-I/O design, Validate
symmetry with recovery, and TLV on-disk notes.
- Planner / NewPlanner / Proof accessor: document the
statelessness rationale.
- Plan: per-bucket classification narrative and the "validate on
every call" trade-off.
- Validate: ordered-checks list with cost annotations.
- validateSweepState: three-state lifecycle diagram.
- Migrate hashSetFromSlice / ensureDisjoint /
confirmedTxidSetContains / allProofTxidsConfirmed /
missingParentsFromSet / ensureParentsConfirmed /
validateSweepState from map[chainhash.Hash]struct{} to
fn.Set[chainhash.Hash] for idiomatic Contains/Intersect usage.
3 participants
Summary
Part 2 of 5 in the stacked split of #235. Adds two pure library
layers that the unroll subsystem builds on:
40582ca—lib/recovery: add recovery proof, session, and state packages. The immutable recovery proof graph (Proof,Node,NodeKind) with topological layering and parent/child tracking. Therecovery
Sessionthat tracks per-node broadcast/confirmation stateand computes CSV maturity.
SessionStatewith custom JSON marshalingfor
map[chainhash.Hash]keys. Proof binary codec for checkpointpersistence. Tree path extraction helpers. Comprehensive unit tests.
d8aa488—unrollplan: add pure dependency-resolution planner.Given a recovery proof, durable progress state, and current block
height, answers which proof transactions are ready to broadcast,
which are blocked by unconfirmed parents, when the target becomes
CSV-mature, and whether the sweep is needed. No I/O, no actors.
Validates state consistency against the proof graph before planning.
Forward-port from the original branch
The original commits imported the legacy
lib/scriptshelper package,which
mainremoved (PR #215). This branch useslib/arkscript.AnchorPkScript/
AnchorOutputinstead. The fixup was folded back into Elle'soriginal commits so each commit compiles standalone — authorship is
preserved on both commits.
Stack
unroll-01-prepunroll-02-planlib/recovery+unrollplanunroll-03-txconfirmtxconfirmactorunroll-04-coreunroll/unroll-05-wireSupersedes #235.
Authorship
Both commits authored by @ellemouton.
Test plan
go test ./lib/recovery/... ./unrollplan/...go vet ./...go build ./cmd/...