Conversation
|
Caution Review failedFailed to post review comments Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR refactors Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/core/application/service.go (1)
4159-4171:script.IsSubDustScriptis dead code in this branch — the inner condition is always true.
IsSubDustScriptrequires an OP_RETURN prefix (per its implementation ininternal/ark-lib/script/script.goLines 125-137). Since this block is only reached whenbytes.HasPrefix(out.PkScript, []byte{txscript.OP_RETURN})returnedfalse,IsSubDustScriptwill always returnfalsehere. The comment "it must be using OP_RETURN-style vtxo pkscript" is therefore unreachable as a passing branch. The guard can be simplified.♻️ Proposed simplification
if out.Value < int64(dust) { - // if the output is below dust limit, it must be using OP_RETURN-style vtxo pkscript - if !script.IsSubDustScript(out.PkScript) { - return nil, errors.AMOUNT_TOO_LOW.New( - "output #%d amount is below dust limit (%d < %d) but is not using "+ - "OP_RETURN output script", outIndex, out.Value, dust, - ).WithMetadata(errors.AmountTooLowMetadata{ - OutputIndex: outIndex, - Amount: int(out.Value), - MinAmount: int(dust), - }) - } + // non-OP_RETURN outputs (not subdust, not asset-packet) must meet the dust floor + return nil, errors.AMOUNT_TOO_LOW.New( + "output #%d amount is below dust limit (%d < %d) but is not using "+ + "OP_RETURN output script", outIndex, out.Value, dust, + ).WithMetadata(errors.AmountTooLowMetadata{ + OutputIndex: outIndex, + Amount: int(out.Value), + MinAmount: int(dust), + }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/application/service.go` around lines 4159 - 4171, The branch's inner script.IsSubDustScript(out.PkScript) check is unreachable because this code path is only entered when bytes.HasPrefix(out.PkScript, []byte{txscript.OP_RETURN}) was false, so IsSubDustScript will always be false; remove the dead check and simplify by directly returning the AMOUNT_TOO_LOW error (update the comment to reflect that non-OP_RETURN outputs below dust are invalid), referencing the symbols out.PkScript, script.IsSubDustScript (to remove), bytes.HasPrefix (reason), and the AMOUNT_TOO_LOW error construction in this block.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/core/application/service.go`:
- Around line 4148-4156: The AmountTooLow error metadata is incorrectly setting
Amount to the minimum threshold (vtxoMinOffchainTxAmount) instead of the actual
output value; in the block that constructs errors.AMOUNT_TOO_LOW.New for the
failing output (referencing out, outIndex, vtxoMinOffchainTxAmount and
AmountTooLowMetadata), change the Amount field to reflect the actual output
value (use out.Value cast to int) while leaving OutputIndex and MinAmount as
they are so the metadata shows the real amount, the min amount, and the output
index.
In `@internal/core/application/validate_outputs_test.go`:
- Around line 214-226: Add an assertion to the failing test that validates the
Amount field in AmountTooLowMetadata so the metadata matches the actual output
value: extend the test case struct with wantAmount (set to 600 for the "reject:
regular output below min amount" scenario) and, when wantErrCode ==
errors.AMOUNT_TOO_LOW.Code, cast the returned error metadata to
AmountTooLowMetadata and assert metadata.Amount == tc.wantAmount (i.e., equals
the out.Value) to catch the bug where Amount was incorrectly set to
vtxoMinOffchainTxAmount.
---
Nitpick comments:
In `@internal/core/application/service.go`:
- Around line 4159-4171: The branch's inner script.IsSubDustScript(out.PkScript)
check is unreachable because this code path is only entered when
bytes.HasPrefix(out.PkScript, []byte{txscript.OP_RETURN}) was false, so
IsSubDustScript will always be false; remove the dead check and simplify by
directly returning the AMOUNT_TOO_LOW error (update the comment to reflect that
non-OP_RETURN outputs below dust are invalid), referencing the symbols
out.PkScript, script.IsSubDustScript (to remove), bytes.HasPrefix (reason), and
the AMOUNT_TOO_LOW error construction in this block.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/core/application/service.gointernal/core/application/validate_outputs_test.go
There was a problem hiding this comment.
♻️ Duplicate comments (2)
internal/core/application/service.go (1)
4148-4156:⚠️ Potential issue | 🟡 Minor
AmountinAmountTooLowMetadatais still set to the minimum threshold, not the actual output value.
Amount: int(vtxoMinOffchainTxAmount)makesAmount == MinAmount, which is meaningless. It should beint(out.Value)— the same pattern used correctly inRegisterIntent(Lines 1748-1757).🐛 Proposed fix
).WithMetadata(errors.AmountTooLowMetadata{ OutputIndex: outIndex, - Amount: int(vtxoMinOffchainTxAmount), + Amount: int(out.Value), MinAmount: int(vtxoMinOffchainTxAmount), })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/application/service.go` around lines 4148 - 4156, The metadata for the AMOUNT_TOO_LOW error is using the minimum threshold instead of the actual output value; update the AmountTooLowMetadata in the error returned by errors.AMOUNT_TOO_LOW.New (the block that checks if out.Value < vtxoMinOffchainTxAmount) so that Amount is set to int(out.Value) (keep MinAmount as int(vtxoMinOffchainTxAmount) and OutputIndex as outIndex) to mirror the correct pattern used in RegisterIntent.internal/core/application/validate_outputs_test.go (1)
214-226:⚠️ Potential issue | 🟡 MinorThe
AMOUNT_TOO_LOWtest case still doesn't assert theAmountmetadata field, so the bug atservice.goline 4154 remains undetected by tests.Adding a
wantAmountfield to the struct and asserting it would have caught theAmount: int(vtxoMinOffchainTxAmount)bug, which setsAmount == MinAmountrather than the actual output value.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/application/validate_outputs_test.go` around lines 214 - 226, Add a new wantAmount field to the test case struct in validate_outputs_test.go for the AMOUNT_TOO_LOW case and set it to the actual output value (e.g., 600); update the test assertion in the test loop to compare the returned metadata.Amount against wantAmount when wantErr is true/false as appropriate so the test will fail if metadata.Amount is incorrectly set to vtxoMinOffchainTxAmount; this will surface the bug at service.go (around line 4154) where Amount is being assigned int(vtxoMinOffchainTxAmount) instead of the actual output value.
🧹 Nitpick comments (2)
internal/core/application/service.go (1)
4159-4171:script.IsSubDustScriptcheck at line 4161 is dead code — simplify the guard.Any script for which
IsSubDustScriptreturnstruestarts withOP_RETURNand is therefore caught by thebytes.HasPrefixbranch at line 4099 (whichcontinues, never reaching this point). By the time execution arrives at line 4159,out.PkScriptis guaranteed to be a non-OP_RETURN, non-anchor script, soIsSubDustScriptalways returnsfalseand!IsSubDustScriptis alwaystrue. The check is vacuous; the error is unconditionally returned wheneverout.Value < int64(dust). The comment "it must be using OP_RETURN-style vtxo pkscript" is also confusing because such outputs have already been handled.♻️ Suggested simplification
- if out.Value < int64(dust) { - // if the output is below dust limit, it must be using OP_RETURN-style vtxo pkscript - if !script.IsSubDustScript(out.PkScript) { - return nil, errors.AMOUNT_TOO_LOW.New( - "output #%d amount is below dust limit (%d < %d) but is not using "+ - "OP_RETURN output script", outIndex, out.Value, dust, - ).WithMetadata(errors.AmountTooLowMetadata{ - OutputIndex: outIndex, - Amount: int(out.Value), - MinAmount: int(dust), - }) - } - } + if out.Value < int64(dust) { + return nil, errors.AMOUNT_TOO_LOW.New( + "output #%d amount is below dust limit (%d < %d)", + outIndex, out.Value, dust, + ).WithMetadata(errors.AmountTooLowMetadata{ + OutputIndex: outIndex, + Amount: int(out.Value), + MinAmount: int(dust), + }) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/application/service.go` around lines 4159 - 4171, The IsSubDustScript check is redundant because any OP_RETURN/anchor scripts are already skipped by the earlier bytes.HasPrefix branch, so when execution reaches the block guarded by out.Value < int64(dust) the output script cannot be OP_RETURN and the script.IsSubDustScript call is dead; remove the script.IsSubDustScript condition and its negation, always return the AMOUNT_TOO_LOW error when out.Value < int64(dust), and update the surrounding comment to state that non-OP_RETURN outputs below dust are invalid; search for symbols out.Value, out.PkScript, script.IsSubDustScript and the earlier bytes.HasPrefix handling to locate and simplify the guard and message.internal/core/application/validate_outputs_test.go (1)
50-421: Missing coverage for theasset.IsAssetPacketbranch.The
asset.IsAssetPacket(out.PkScript)branch atservice.goline 4108 — which adds the output directly to the result list and skips all amount/dust checks — has no test case. A test with an asset-packet OP_RETURN output (possibly also testing that it is not blocked by the amount guards and thatfoundOpReturnis set so a second OP_RETURN is rejected) would close this gap.Would you like me to draft the missing test cases for the asset packet path?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/application/validate_outputs_test.go` around lines 50 - 421, Add tests to TestValidateOffchainTxOutputs that cover the asset packet OP_RETURN branch: create a txOut with a PkScript that asset.IsAssetPacket would return true for (e.g., add a helper like testAssetPacketScript and use it), call validateOffchainTxOutputs with vtxoMaxAmount/vtxoMinOffchainTxAmount set to values that would normally reject a regular output, and assert the call succeeds and includes the asset output in outputs (wantOutputCount). Also add a second test that includes an asset packet OP_RETURN plus another OP_RETURN to verify the duplicate OP_RETURN path still triggers the MALFORMED_ARK_TX error (ensure you check err.Code() and err.Error() contains "multiple op return"). Ensure tests reference TestValidateOffchainTxOutputs, validateOffchainTxOutputs, and asset.IsAssetPacket to locate the logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@internal/core/application/service.go`:
- Around line 4148-4156: The metadata for the AMOUNT_TOO_LOW error is using the
minimum threshold instead of the actual output value; update the
AmountTooLowMetadata in the error returned by errors.AMOUNT_TOO_LOW.New (the
block that checks if out.Value < vtxoMinOffchainTxAmount) so that Amount is set
to int(out.Value) (keep MinAmount as int(vtxoMinOffchainTxAmount) and
OutputIndex as outIndex) to mirror the correct pattern used in RegisterIntent.
In `@internal/core/application/validate_outputs_test.go`:
- Around line 214-226: Add a new wantAmount field to the test case struct in
validate_outputs_test.go for the AMOUNT_TOO_LOW case and set it to the actual
output value (e.g., 600); update the test assertion in the test loop to compare
the returned metadata.Amount against wantAmount when wantErr is true/false as
appropriate so the test will fail if metadata.Amount is incorrectly set to
vtxoMinOffchainTxAmount; this will surface the bug at service.go (around line
4154) where Amount is being assigned int(vtxoMinOffchainTxAmount) instead of the
actual output value.
---
Nitpick comments:
In `@internal/core/application/service.go`:
- Around line 4159-4171: The IsSubDustScript check is redundant because any
OP_RETURN/anchor scripts are already skipped by the earlier bytes.HasPrefix
branch, so when execution reaches the block guarded by out.Value < int64(dust)
the output script cannot be OP_RETURN and the script.IsSubDustScript call is
dead; remove the script.IsSubDustScript condition and its negation, always
return the AMOUNT_TOO_LOW error when out.Value < int64(dust), and update the
surrounding comment to state that non-OP_RETURN outputs below dust are invalid;
search for symbols out.Value, out.PkScript, script.IsSubDustScript and the
earlier bytes.HasPrefix handling to locate and simplify the guard and message.
In `@internal/core/application/validate_outputs_test.go`:
- Around line 50-421: Add tests to TestValidateOffchainTxOutputs that cover the
asset packet OP_RETURN branch: create a txOut with a PkScript that
asset.IsAssetPacket would return true for (e.g., add a helper like
testAssetPacketScript and use it), call validateOffchainTxOutputs with
vtxoMaxAmount/vtxoMinOffchainTxAmount set to values that would normally reject a
regular output, and assert the call succeeds and includes the asset output in
outputs (wantOutputCount). Also add a second test that includes an asset packet
OP_RETURN plus another OP_RETURN to verify the duplicate OP_RETURN path still
triggers the MALFORMED_ARK_TX error (ensure you check err.Code() and err.Error()
contains "multiple op return"). Ensure tests reference
TestValidateOffchainTxOutputs, validateOffchainTxOutputs, and
asset.IsAssetPacket to locate the logic.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/core/application/service.gointernal/core/application/validate_outputs_test.go
![arkanaai[bot]](https://avatars.githubusercontent.com/u/170820085?s=60&v=4){kind=small}
There was a problem hiding this comment.
arkanaai[bot] review — arkd #925
Solid security hardening. Extracts validateOffchainTxOutputs into a testable standalone function and adds the critical missing guards on OP_RETURN outputs.
New guards — correct ✓
-
Subdust OP_RETURN with value ≥ dust →
MALFORMED_ARK_TX: Previously, a malicious client could craft a subdust-format OP_RETURN with dust-or-higher satoshis and bypass thevtxoMinOffchainTxAmountfloor check (the code only checked the< dustpath for non-OP_RETURN outputs). Now explicitly blocked. ✓ -
Non-subdust OP_RETURN with non-zero value →
MALFORMED_ARK_TX: Standard OP_RETURN outputs in Bitcoin carry 0 sats; a non-zero value here indicates a malformed or adversarial transaction. ✓ -
Asset packet OP_RETURN outputs are correctly passed through without the above value checks (asset packets carry their own semantics). ✓
Test coverage is comprehensive — 421 lines covering all cases including anchor duplicate, OP_RETURN duplicate, all the new subdust guards, and the standard amount bounds.
arkd #923 (also open, reviewed this cycle) added commit 23a65815 which renames vtxoMinOffchainTxAmount → vtxoMinAmount and removes vtxoMinSettlementAmount from the struct. This PR's call site still uses s.vtxoMinOffchainTxAmount:
outputs, outputsErr := validateOffchainTxOutputs(
..., s.vtxoMinOffchainTxAmount, ...
)These two PRs will conflict when either is merged. Whichever merges second will need to rebase and update the field reference. The extracted function signature (vtxoMinOffchainTxAmount int64 param) should also be renamed to vtxoMinAmount after #923 lands.
Minor — foundAnchor check moved to end
The anchor check is now at the bottom of the loop (after all output validation). Previously it short-circuited early. The behavior is equivalent — we still reject if no anchor is found — but a transaction with a missing anchor AND a malformed OP_RETURN output will now return the OP_RETURN error, not the missing-anchor error. This changes the error priority order; not a security issue but worth being aware of for client error message handling.
bitcoin-coder-bob
force-pushed
the
bob/ensure-subdust-guards
branch
from
ab31d0d to
f7265cc
Compare
…guards # Conflicts: # internal/core/application/service.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/core/application/service.go`:
- Around line 4230-4264: The extension OP_RETURN path currently skips the
non-zero-value check because extension.IsExtension(out.PkScript) runs before
value validation; modify the logic in the output-validation block (where
extension.IsExtension, extension.NewExtensionFromBytes, script.IsSubDustScript
and errors.MALFORMED_ARK_TX are used) to enforce out.Value == 0 for non-subdust
OP_RETURNs before or alongside parsing extensions (i.e., if not
script.IsSubDustScript(out.PkScript) and out.Value > 0 return MALFORMED_ARK_TX),
and only then call extension.NewExtensionFromBytes for extension scripts; also
add a regression test in validate_outputs_test.go that constructs an extension
OP_RETURN with a non-zero value and asserts the MALFORMED_ARK_TX error is
returned.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/core/application/service.gointernal/core/application/validate_outputs_test.go
| // if the OP_RETURN is extension, decode it and add it to outputs list | ||
| // skip other checks related to vtxo output | ||
| if extension.IsExtension(out.PkScript) { | ||
| outputs = append(outputs, out) | ||
|
|
||
| var err error | ||
| ext, err = extension.NewExtensionFromBytes(out.PkScript) | ||
| if err != nil { | ||
| return nil, nil, errors.MALFORMED_ARK_TX.New( | ||
| "tx %s has malformed extension output %x", txid, out.PkScript, | ||
| ).WithMetadata(errors.PsbtMetadata{Tx: signedArkTx}) | ||
| } | ||
| continue | ||
| } | ||
|
|
||
| // subdust OP_RETURN: must have value < dust | ||
| if script.IsSubDustScript(out.PkScript) { | ||
| if out.Value >= int64(dust) { | ||
| return nil, nil, errors.MALFORMED_ARK_TX.New( | ||
| "subdust OP_RETURN output #%d has value (%d) >= dust limit (%d)", | ||
| outIndex, out.Value, dust, | ||
| ).WithMetadata(errors.PsbtMetadata{Tx: signedArkTx}) | ||
| } | ||
| outputs = append(outputs, out) | ||
| continue | ||
| } | ||
|
|
||
| // not subdust format but has value in invalid | ||
| if out.Value > 0 { | ||
| return nil, nil, errors.MALFORMED_ARK_TX.New( | ||
| "OP_RETURN output #%d has non-zero value (%d) but is not a subdust output", | ||
| outIndex, out.Value, | ||
| ).WithMetadata(errors.PsbtMetadata{Tx: signedArkTx}) | ||
| } | ||
| outputs = append(outputs, out) |
There was a problem hiding this comment.
Extension OP_RETURN currently bypasses the non-zero-value OP_RETURN guard.
extension.IsExtension(out.PkScript) is checked before the non-subdust OP_RETURN value check, so an extension OP_RETURN with out.Value > 0 is accepted. That defeats the new “non-subdust OP_RETURN must be zero” rule.
Please enforce the value guard before (or alongside) extension parsing, and add a regression case in internal/core/application/validate_outputs_test.go for “extension OP_RETURN with non-zero value”.
🐛 Proposed fix
// verify we don't have multiple OP_RETURN outputs
if bytes.HasPrefix(out.PkScript, []byte{txscript.OP_RETURN}) {
if foundOpReturn {
return nil, nil, errors.MALFORMED_ARK_TX.New(
"tx %s has multiple op return outputs", txid,
).WithMetadata(errors.PsbtMetadata{Tx: signedArkTx})
}
foundOpReturn = true
- // if the OP_RETURN is extension, decode it and add it to outputs list
- // skip other checks related to vtxo output
- if extension.IsExtension(out.PkScript) {
- outputs = append(outputs, out)
-
- var err error
- ext, err = extension.NewExtensionFromBytes(out.PkScript)
- if err != nil {
- return nil, nil, errors.MALFORMED_ARK_TX.New(
- "tx %s has malformed extension output %x", txid, out.PkScript,
- ).WithMetadata(errors.PsbtMetadata{Tx: signedArkTx})
- }
- continue
- }
-
// subdust OP_RETURN: must have value < dust
- if script.IsSubDustScript(out.PkScript) {
+ if script.IsSubDustScript(out.PkScript) {
if out.Value >= int64(dust) {
return nil, nil, errors.MALFORMED_ARK_TX.New(
"subdust OP_RETURN output #%d has value (%d) >= dust limit (%d)",
outIndex, out.Value, dust,
).WithMetadata(errors.PsbtMetadata{Tx: signedArkTx})
}
- outputs = append(outputs, out)
- continue
- }
-
- // not subdust format but has value in invalid
- if out.Value > 0 {
+ } else if out.Value > 0 {
return nil, nil, errors.MALFORMED_ARK_TX.New(
"OP_RETURN output #%d has non-zero value (%d) but is not a subdust output",
outIndex, out.Value,
).WithMetadata(errors.PsbtMetadata{Tx: signedArkTx})
}
+
+ if extension.IsExtension(out.PkScript) {
+ var err error
+ ext, err = extension.NewExtensionFromBytes(out.PkScript)
+ if err != nil {
+ return nil, nil, errors.MALFORMED_ARK_TX.New(
+ "tx %s has malformed extension output %x", txid, out.PkScript,
+ ).WithMetadata(errors.PsbtMetadata{Tx: signedArkTx})
+ }
+ }
outputs = append(outputs, out)
continue
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/core/application/service.go` around lines 4230 - 4264, The extension
OP_RETURN path currently skips the non-zero-value check because
extension.IsExtension(out.PkScript) runs before value validation; modify the
logic in the output-validation block (where extension.IsExtension,
extension.NewExtensionFromBytes, script.IsSubDustScript and
errors.MALFORMED_ARK_TX are used) to enforce out.Value == 0 for non-subdust
OP_RETURNs before or alongside parsing extensions (i.e., if not
script.IsSubDustScript(out.PkScript) and out.Value > 0 return MALFORMED_ARK_TX),
and only then call extension.NewExtensionFromBytes for extension scripts; also
add a regression test in validate_outputs_test.go that constructs an extension
OP_RETURN with a non-zero value and asserts the MALFORMED_ARK_TX error is
returned.
There was a problem hiding this comment.
@bitcoin-coder-bob the rabbit is right, for extension let's check value = 0, fail otherwise (it may change later if we include subdust to extension output but in the meantime better to be explicit). We should also add this check to RegisterIntent, cause the intent may contain extension
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| @@ -0,0 +1,428 @@ | |||
| package application | |||
There was a problem hiding this comment.
let's move this test to service_test.go
2 participants
Closes #841
To be rebased off of #923 after it is merged first
Created a functions
validateOffchainTxOutputscalled from within existingSubmitOffchainTxto separate out the validation logic. Within this logic is the addition of the 2 new guards outlined in the issue.Tests were added against this new function in the new test file:
internal/core/application/validate_outputs_test.goSummary by CodeRabbit
Refactor
Tests