Repository navigation
chore: update regtest to latest, emulator v0.0.7 support - #192
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 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 |
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Review
Two changes: removal of ARK_CONTAINER=ark from .env.regtest, and a regtest submodule bump (15354f99 → 56dca6ed).
ARK_CONTAINER=ark removal — safe
The removed line was inert. Tracing both consumers:
- regtest submodule (
regtest.mjs:307): hardcodesdocker exec arkd …—ARK_CONTAINERis never read. - dotnet tests (
DockerHelper.cs:477):Container.Arkd = "arkd"— also hardcoded, never reads the env var.
The compose stack (docker/compose.ark.yml) fixes container_name: arkd, so the override to "ark" would have been actively wrong if anything had read it. The accompanying comment ("SDK tests hardcode 'ark' for docker exec") was stale — the tests hardcode "arkd", not "ark". Removal is correct.
Submodule bump — looks fine
The bump pulls in update .env.defaults (#57), which advances ARKD_IMAGE and ARKD_WALLET_IMAGE to v0.9.16 (from v0.9.15 in the outgoing commit). No structural changes to compose topology, container names, or ports that would affect test harness assumptions. No cross-repo API surface is touched.
No protocol-critical paths involved
This change is entirely infrastructure/config. VTXO handling, signing, forfeit, round lifecycle, and unilateral exit paths are unaffected. No concerns.
Emulator v0.0.7 (arkade-os/emulator#129, "validate checkpoints and prevouts") walks every input of a submitted Arkade transaction or intent proof and hard-fails on a missing prevarktx field; v0.0.6 only validated the fields a PSBT happened to carry, so zero fields meant zero checks. The SDK never attached the field, so once the regtest submodule bumped the emulator to v0.0.7 every POST /v1/tx failed with "missing prevout tx for input 0". The transaction an Arkade input must carry is the checkpoint's parent, not the checkpoint: input i spends checkpoint i, which spends the coin, so it is the VTXO's own creating transaction. The emulator reconciles it against the checkpoint's witness utxo. Only the previous transaction's outputs are read (txid, value, script, introspection opcodes), all witness-independent, so a PSBT's unsigned global transaction resolves correctly and no signature propagation has to be waited on. The field also lives in the PSBT unknown map, which no sighash covers, so it is attached after signing, at submit time. - IPrevArkTxProvider / PrevArkTxProvider resolve previous transactions from IVirtualTxStorage, then arkd's indexer, then chain. Indexer results are keyed by the txid parsed out of each body, since arkd returns them in database order rather than request order, and bodies that were not requested are dropped. - ArkadePsbtExtensions.AttachPrevArkTxsAsync / AttachIntentPrevArkTxsAsync annotate an Arkade transaction and a BIP322 intent proof respectively; the proof's synthetic input 0 is left alone. An input already carrying the field keeps it, since the emulator rejects an input bearing two. - ArkadeEmulatorSpendSubmitter annotates before submitting; AddArkadeEmulator registers the provider. - IBitcoinBlockchain.GetRawTransactionAsync, defaulted to NotSupportedException and implemented for Esplora and NBXplorer. Boarding and commitment parents have no off-chain body, so an intent proof registering a boarding input cannot be annotated without it. Unresolvable parents throw with the input index and txid named rather than letting the emulator reject the submission. The batch tree-signing hop still submits to POST /v1/tx with an empty checkpoint list, which v0.0.7 rejects before it looks at prevouts; that needs a different shape and is documented as a limit.
d4rp4t
changed the title
8c20170 commented out the e2e-swaps job but left it in publish's needs list, so Actions rejected the whole workflow file — "Job 'publish' depends on unknown job 'e2e-swaps'" — and CI failed at startup in 0s without running a single job.
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
PROTOCOL-CRITICAL: human review required.
Status: 2 commits since last pass (infra-only: .env.regtest + submodule bump). Prior review raised 0 protocol findings — nothing to close out. This pass covers the substantive new work: emulator v0.0.7 prevarktx support.
Prior findings
None open. Previous review covered .env.regtest removal and submodule bump only. All clean.
New findings
1. RpcBlockchain does not override GetRawTransactionAsync — doc claim is wrong [Bug]
NArk.Core/Blockchain/RpcBlockchain.cs implements IBitcoinBlockchain but has no GetRawTransactionAsync override (confirmed: 193-line file, public methods end at EstimateFeeRateAsync). The new interface default (IBitcoinBlockchain.cs:120-122) throws NotSupportedException.
The interface XML doc states: "every in-box backend overrides it" — that is incorrect; RpcBlockchain is an in-box backend and does not.
What happens at runtime: PrevArkTxProvider.ResolveAsync catches Exception when e is not OperationCanceledException on the per-txid chain call (IPrevArkTxProvider.cs chain loop). NotSupportedException matches that guard, gets logged at Debug, and is silently treated as a miss. AttachResolvedAsync then throws InvalidOperationException naming the input index and txid — which is correct behaviour (fail before submit, not at the emulator) but the debug log gives no indication the real cause is a missing override, making this hard to diagnose in the field.
RpcBlockchain already holds an RPCClient; the fix is a direct lift from NBXplorerBlockchain.GetRawTransactionAsync (same getrawtransaction / verbose=false call pattern). Fix the implementation or correct the doc.
2. ArkadeBatchSessionExtension.CoSignAsync is broken against emulator v0.0.7, with no code guard [Protocol]
ArkadeBatchSessionExtension.cs routes PostTreeSigning through CoSignWithEmulatorAsync, which:
- passes an empty checkpoint list to
POST /v1/tx - does not call
AttachPrevArkTxsAsync, so noprevarktxfields are set
Emulator v0.0.7 requires exactly one checkpoint per input and the prevarktx field on every input before it inspects anything else. Both are absent. The emulator will reject with missing prevout tx for input N (or a checkpoint count error) on the first submission.
The docs acknowledge this (docs/articles/arkade-script.md, last bullet): "The batch tree-signing hop does not carry checkpoints… That hop therefore needs the checkpoint-carrying shape (or its own endpoint) before it can work against a current emulator." But there is no code gate: a caller who wires AddArkadeEmulator and runs a batch against a v0.0.7 emulator will reach the emulator with a malformed submission. The failure surfaces at the emulator, not at registration time.
Recommend adding a throw new NotSupportedException(...) in CoSignAsync at PostTreeSigning (analogous to the existing guard at PreForfeitFinalization) until the endpoint shape is resolved. The forfeit guard's rationale — "failing loudly beats silently producing an invalid result" — applies equally here.
3. CoSignWithEmulatorAsync is silently incompatible with emulator v0.0.7 [Minor]
ArkadePsbtExtensions.CoSignWithEmulatorAsync is public API. It does not call AttachPrevArkTxsAsync. Any caller who uses this helper directly against a v0.0.7 emulator will be rejected with missing prevout tx for input N. The README now documents the manual call pattern (await arkTx.AttachPrevArkTxsAsync(...) before CoSignWithEmulatorAsync) but the method's XML doc has no mention of this.
Add a <para> note: emulator v0.0.7+ requires prevarktx on every input; call AttachPrevArkTxsAsync / AttachIntentPrevArkTxsAsync before this method if the submission target is v0.0.7+.
4. e2e-swaps CI job silently disabled [Tracking]
.github/workflows/build.yml: e2e-swaps job is commented out as "deprecated". If swap paths covered any forfeit exit or round-lifecycle scenarios, those are now uncovered in CI. Please ensure the intent is tracked — deprecating a CI job without a replacement leaves a coverage gap that won't be noticed until a regression ships.
What looks good
Protocol logic in AttachPrevArkTxsAsync: The indirection is correct — prevarktx on Arkade input i is the transaction funding the checkpoint's single input (the VTXO/boarding tx), not the checkpoint itself. The emulator holds the checkpoint and uses prevarktx to validate the checkpoint's spend. The code correctly resolves this via checkpoint.Inputs[0].PrevOut.Hash.
AttachIntentPrevArkTxsAsync: Correctly skips input 0 (BIP322 message input, emulator synthesises its prevout) and annotates inputs 1..N directly from their PrevOut.Hash.
Response ordering defence in PrevArkTxProvider: arkd's GetVirtualTxs returns hexes in DB order, not request order. The code keys each result by the txid parsed from the body, not by position in the response array — correct and tested (Provider_KeysIndexerResultsByParsedTxid_NotRequestOrder).
Supply-chain defence: wanted.Contains(txid) ensures a server responding with an unrequested transaction cannot attach it to an input. Tested (Provider_DropsTransactionsThatWereNotRequested).
Txid verification in GetRawTransactionAsync (Esplora + NBXplorer): Both implementations parse the response and verify tx.GetHash() == txid rather than trusting the label on the response. Correct.
Test coverage (PrevArkTxTests.cs, 375 lines): All critical paths covered — happy path, signature preservation, ordering defence, storage priority, chain fallback, indexer failure with chain fallback, indexer failure without chain fallback, idempotent field, serialisation format (no segwit marker in prevarktx value), unparseable hex tolerance. The Danger "source changed with no test changes" warning appears to have been evaluated before PrevArkTxTests.cs was counted.
IBitcoinBlockchain.GetRawTransactionAsync default: Throwing NotSupportedException with the type name in the message is the right default — custom implementors get a clear error at the point of failure. The TryAddSingleton guard in EmulatorServiceCollectionExtensions correctly prevents double-registration.
PSBT mutation + NSubstitute assertion in SpendSubmitter_SubmitAsync_ForwardsArkTxAndCheckpointsToEmulator: arkTx.ToBase64() is evaluated after AttachPrevArkTxsAsync mutates it in place, so the assertion correctly matches what was submitted (annotated PSBT). The added Assert.That(arkTx.Inputs[0].GetArkFieldPrevArkTx(...)) separately confirms the mutation occurred. Both correct.
RpcBlockchain did not override GetRawTransactionAsync, contradicting the interface doc's claim that every in-box backend does. It holds an RPCClient already, so it gets the same getrawtransaction/verbose=false implementation as the NBXplorer backend. A backend that genuinely cannot serve raw transactions was swallowed per-txid at debug level, so the caller's "could not be resolved" error named the symptom and hid the cause. PrevArkTxProvider now reports a NotSupportedException once, at warning level, naming the backend and the missing override, and stops retrying the remaining txids. ArkadeBatchSessionExtension routed PostTreeSigning to POST /v1/tx with an empty checkpoint list and no prevarktx fields. Emulator v0.0.7 requires one checkpoint per input plus the field on every input before it inspects anything else, and a tree transaction has neither, so the hop reached the emulator with a request it cannot accept. It now throws NotSupportedException at the call site, matching the existing PreForfeitFinalization guard. CoSignWithEmulatorAsync attaches no prevarktx and submits no checkpoints, so it is subject to the same rejection. Its XML doc now says so and points at AttachPrevArkTxsAsync / AttachIntentPrevArkTxsAsync.
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
PROTOCOL-CRITICAL: human review required.
Status: 2 commits since last pass (fixing 3 gaps in emulator v0.0.7 support). All 4 prior findings closed. 1 dead-code observation and 2 minor concerns are new.
Prior findings
1. RpcBlockchain missing GetRawTransactionAsync override — Fixed
The override is now present (RpcBlockchain.cs:172–204) using the same getrawtransaction/verbose=false pattern as GetTxStatusAsync, with txid verification. The IBitcoinBlockchain doc is corrected: "the in-box Esplora, NBXplorer and Bitcoin Core RPC backends all override it."
2. ArkadeBatchSessionExtension.CoSignAsync broken for PostTreeSigning — Fixed
PostTreeSigning now throws NotSupportedException at ArkadeBatchSessionExtension.cs:102–113 before the emulator is contacted. The guard is placed after PreForfeitFinalization (correct order) and includes the checkpoint-shape rationale. The class-level XML doc is updated to reflect both phases throwing. Test replaced by CoSign_RefusesTreeTransactions_RatherThanSubmittingAMalformedRequest, which asserts the exception and confirms SubmitTxAsync is never called.
3. CoSignWithEmulatorAsync XML doc silent on prevarktx — Fixed
The added <para> warns callers that emulator v0.0.7+ requires prevarktx on every input and points at AttachPrevArkTxsAsync/AttachIntentPrevArkTxsAsync. It also flags the empty checkpoint list limitation.
4. e2e-swaps CI job silently disabled — No longer applicable
Removing e2e-swaps from publish.needs is the correct follow-through to the deprecation already in the previous commit. The underlying coverage gap was noted in the last review; nothing new here.
New findings
1. Emulator signing loop in CoSignAsync is unreachable dead code [Minor]
ArkadeBatchSessionExtension.cs:116–130 — the signed = new PSBT[psbts.Count]; for (var i …) loop is unreachable. BatchExtensionPhase has exactly two values (PostTreeSigning = 1, PreForfeitFinalization = 2), both of which throw before reaching this code. The path through the loop would require an arkade-bound input set and a phase that is neither of those two, which cannot happen with the current enum.
The code isn't harmful, but it is confusing: a maintainer adding a new phase later might not realise the loop is already there, or might rely on it without understanding it was never exercised. Options: remove the loop and leave a comment saying "add the submission loop here when a submittable phase is introduced", or add a default: throw new ArgumentOutOfRangeException(nameof(phase)) above it to make the unreachability explicit.
2. Network.Main hardcoded in RpcBlockchain.GetRawTransactionAsync [Minor]
RpcBlockchain.cs:195: Transaction.Parse(hex, Network.Main). The RPCClient already carries _client.Network, so the network the backend is connected to is available. The comment acknowledges this is "network-independent" for serialisation purposes and the txid verification (tx.GetHash() == txid) prevents the function from returning a wrong result. In practice this is safe. Consistency and explicitness favour _client.Network — one word change.
3. Provider_BackendWithoutRawTxSupport_IsAMissNotACrash only covers single-txid case [Minor]
PrevArkTxTests.cs — the test uses a single txid, so whether break fires or not doesn't affect the outcome. The valuable property of the new break (one warning for N txids, no per-txid retry storm when the backend structurally cannot serve raw transactions) is not covered. This is low priority but worth a follow-up test case with two wanted txids confirming only one warning is emitted and both are absent from resolved.
What looks good
PrevArkTxProvider.NotSupportedException handler (IPrevArkTxProvider.cs:151–163): catches before the general Exception guard, logs at Warning (not Debug) naming the backend type and the remediation (override GetRawTransactionAsync), then breaks to avoid per-txid repetition. Exactly right.
RpcBlockchain implementation details: verbose=false returns raw hex (not a decoded JSON object), response.Error is not null checked before parsing, txid verified against the returned transaction hash — matches the Esplora/NBXplorer pattern and the security rationale from the last review.
Test CoSign_RefusesTreeTransactions_*: Asserts message contains "checkpoint" (correct signal) and explicitly confirms SubmitTxAsync was never invoked. Removing CoSign_PropagatesEmulatorFailure is correct — that test exercised the emulator loop which is now unreachable.
docs/articles/arkade-script.md: Updated correctly: the two-bullet "not implemented" notes are merged into a single accurate description covering both phases and both reasons.
…rdcoded networks CoSignAsync's per-PSBT emulator loop was unreachable: BatchExtensionPhase has two values and both throw above it. A maintainer adding a third phase would have found a loop that had never run. The two guards become a switch over the phase whose default arm throws ArgumentOutOfRangeException, so an unhandled phase fails loudly and the place a future submission arm belongs is named rather than implied. RpcBlockchain and NBXplorerBlockchain both know the network they are connected to, so they parse against it instead of Network.Main. Esplora is constructed from a bare base URI and has none; its comment now says that rather than reading as an oversight. The backend-without-raw-tx-support test used a single txid, so it could not tell whether the loop gives up after the first structural failure. A second case uses two txids and asserts one chain call and one warning.
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
PROTOCOL-CRITICAL: human review required.
Status: 1 commit since last pass (c6cf291 — refactor: make unreachable co-sign path explicit, drop hardcoded networks). All 3 prior findings fixed. 0 new findings.
Prior findings
1. Dead-code unreachable signing loop — Fixed (c6cf291)
The loop is removed entirely. The two sequential if (phase ==) guards are consolidated into a single throw phase switch { ..., _ => new ArgumentOutOfRangeException(...) } expression. The _ arm carries an explicit message guiding whoever adds the next phase, which is exactly the remediation suggested. Clean.
2. Network.Main hardcoded in RpcBlockchain and NBXplorerBlockchain — Fixed (c6cf291)
RpcBlockchain.cs:193 now uses _client.Network; NBXplorerBlockchain.cs:288 uses _explorerClient.Network.NBitcoinNetwork. Both are the right references for their respective client types. EsploraBlockchain retains Network.Main with an updated comment explaining the constraint (constructed from a bare URI — no network object available); that is correct.
3. Provider_BackendWithoutRawTxSupport_IsAMissNotACrash only covered the single-txid case — Fixed (c6cf291)
New test Provider_BackendWithoutRawTxSupport_GivesUpAfterTheFirstTxid (PrevArkTxTests.cs:294–327) supplies two txids, asserts chain.Received(1).GetRawTransactionAsync(...) (exactly one RPC call — the break fires), and asserts logger.Received(1).Log(LogLevel.Warning, ..., Arg.Any<NotSupportedException>(), ...) (exactly one warning, carrying the exception). Both assertions are correct against the NSubstitute ILogger pattern.
What looks good in this commit
Switch consolidation in CoSignAsync: Collapsing the two guards into throw phase switch { ..., _ => new ArgumentOutOfRangeException(...) } is strictly better — the _ arm makes exhaustiveness explicit, the message tells the next developer exactly what to do, and the dead signing loop is gone rather than just commented. No behaviour change for existing callers.
_emulator field retention comment: The inline comment explaining why the field stays even though nothing currently submits is accurate and saves the next reviewer from flagging it as dead code.
Esplora Network.Main comment: The revised comment ("Unlike the RPC and NBXplorer backends, this one is constructed from a bare base URI and so has no network to hand") correctly explains the asymmetry rather than repeating the prior vague claim about network-independence.
No open findings remain. The three refactors in this commit are mechanical, correct, and well-tested.
No description provided.