vtxoMinOffchainTxAmount clamped to dust when below it#923
vtxoMinOffchainTxAmount clamped to dust when below it#923bitcoin-coder-bob wants to merge 7 commits intomasterfrom
Conversation
…s also clamped to dust when below it
WalkthroughConsolidates VTXO/UTXO minimum handling into a unified internal vtxoMinAmount and dustAmount resolved via resolveMinAmounts; updates service startup, validation paths, and ServiceInfo exposure; adds unit tests for the resolver. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/core/application/utils.go (1)
535-537: Consider guarding against all negativevtxoMinOffchainvalues, not just-1.The
== -1sentinel check only handles the "unset" case. For anyvtxoMinAmount < -1(e.g.,-50),vtxoMinOffchainretains the negative value. InSubmitOffchainTx,out.Value < s.vtxoMinOffchainTxAmountthen never fires for any valid Bitcoin output, silently bypassing the configured minimum. The dust + OP_RETURN check at line 970 still applies, but the operator-configured floor is lost.If values below
-1are prohibited by upstream config validation, a one-line comment confirming this would prevent future confusion. Otherwise, consider:♻️ Suggested guard
-if vtxoMinOffchain == -1 { +if vtxoMinOffchain < 0 { vtxoMinOffchain = dustAmount }Only if intentional support for explicit sub-zero values is not needed; otherwise document the invariant.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/application/utils.go` around lines 535 - 537, The check for vtxoMinOffchain currently only treats -1 as the "unset" sentinel, which allows other negative values to persist and bypass the configured minimum; update the initialization logic around vtxoMinOffchain (the block that sets it to dustAmount) to guard against any negative value (e.g., if vtxoMinOffchain < 0 then set to dustAmount) so SubmitOffchainTx's comparison against s.vtxoMinOffchainTxAmount works correctly, or alternatively add a concise comment on the invariant that upstream validation prevents values < -1 if you intentionally want to allow distinct negative sentinels. Ensure references include vtxoMinOffchain, vtxoMinOffchainTxAmount, SubmitOffchainTx, and dustAmount so reviewers can locate the change.internal/core/application/service_test.go (1)
64-127: Well-structured tests; consider adding a case forvtxoMinAmount < -1.All 5 cases are logically correct and consistent with the implementation. One coverage gap: there is no case for
vtxoMinAmount = -50(negative, but not the-1sentinel). Given the== -1guard inresolveMinAmounts, such input would leavevtxoMinOffchain = -50, which is the edge case discussed inutils.go. Adding a case here would document — and pin — the current behavior explicitly:🧪 Suggested additional test case
+{ + description: "other negative: settlement clamped, offchain preserved as-is", + vtxoMinAmount: -50, + utxoMinAmount: -50, + expectedVtxoSettlement: dust, + expectedVtxoOffchain: -50, + expectedUtxoMin: dust, +},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/application/service_test.go` around lines 64 - 127, Add a test case to TestResolveMinAmounts to cover vtxoMinAmount < -1 (e.g., -50) so the behavior where resolveMinAmounts preserves negative offchain values except for the -1 sentinel is pinned; specifically, add an entry in testCases with description like "vtxoMinAmount < -1 preserves offchain negative", vtxoMinAmount: -50, utxoMinAmount: -1 (or another appropriate utxo input), and set expectedVtxoSettlement to dust, expectedVtxoOffchain to -50, and expectedUtxoMin to dust to assert resolveMinAmounts returns the clamped settlement, preserves the negative offchain, and clamps utxoMin.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/core/application/service_test.go`:
- Around line 64-127: Add a test case to TestResolveMinAmounts to cover
vtxoMinAmount < -1 (e.g., -50) so the behavior where resolveMinAmounts preserves
negative offchain values except for the -1 sentinel is pinned; specifically, add
an entry in testCases with description like "vtxoMinAmount < -1 preserves
offchain negative", vtxoMinAmount: -50, utxoMinAmount: -1 (or another
appropriate utxo input), and set expectedVtxoSettlement to dust,
expectedVtxoOffchain to -50, and expectedUtxoMin to dust to assert
resolveMinAmounts returns the clamped settlement, preserves the negative
offchain, and clamps utxoMin.
In `@internal/core/application/utils.go`:
- Around line 535-537: The check for vtxoMinOffchain currently only treats -1 as
the "unset" sentinel, which allows other negative values to persist and bypass
the configured minimum; update the initialization logic around vtxoMinOffchain
(the block that sets it to dustAmount) to guard against any negative value
(e.g., if vtxoMinOffchain < 0 then set to dustAmount) so SubmitOffchainTx's
comparison against s.vtxoMinOffchainTxAmount works correctly, or alternatively
add a concise comment on the invariant that upstream validation prevents values
< -1 if you intentionally want to allow distinct negative sentinels. Ensure
references include vtxoMinOffchain, vtxoMinOffchainTxAmount, SubmitOffchainTx,
and dustAmount so reviewers can locate the change.
![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 #923
Clean refactor that extracts dust-clamping logic into a testable resolveMinAmounts helper. Good test coverage across all edge cases (below-dust, zero, -1, at-dust, above-dust).
Protocol correctness ✓
The behavioral change is intentional and clearly documented: vtxoMinOffchainTxAmount is not clamped to dust when the operator sets it below dust — sub-dust offchain VTXOs are valid (OP_RETURN scripts). Only the settlement minimum is clamped. This is correct per Ark protocol semantics.
GetInfo.VtxoMinAmount
// service.go:2083
- VtxoMinAmount: s.vtxoMinOffchainTxAmount,
+ VtxoMinAmount: s.vtxoMinSettlementAmount,This changes what VtxoMinAmount means in the public GetInfo RPC response. Before: the minimum for offchain transactions. After: the minimum for settlement (always at least dust). SDK clients that read GetInfo().VtxoMinAmount to gate their send UI will now see a potentially larger value than before (if the operator configured a sub-dust offchain minimum). This is almost certainly the correct behavior (you want the UI to show the settlement minimum), but:
- Confirm this matches what go-sdk, ts-sdk, and rust-sdk expect from this field
- Consider whether this needs a proto comment update to clarify the semantic distinction between "offchain min" and "settlement min"
Otherwise looks good. The test at service_test.go:62-131 is thorough and directly verifies the invariants.
|
@bitcoin-coder-bob let's instead drop the config for min/max settlement amount completely and keep returning the one for offchain txs (rename them vtxoMinAmount/vtxoMaxAmount if you want). The min amount for settlement will always be DUST value (330), no other config env var needed for that |
|
arkanaai[bot] follow-up — arkd #923 New commit The architecture is now simpler: a single arkd #925 (reviewed this cycle) was written against the version of this PR that still had // arkd #925 call site (needs update after #923 lands):
outputs, outputsErr := validateOffchainTxOutputs(
..., s.vtxoMinOffchainTxAmount, ...
// ^^^^^^^^^^^^^^^^^^^^^^^^
// must become s.vtxoMinAmount
)Merge order suggestion: land #923 first, then rebase #925 on top. #925 only needs a one-line rename at the call site and in the function signature. |
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 957-965: The current vtxoMinAmount check (using s.vtxoMinAmount
against out.Value and returning errors.AMOUNT_TOO_LOW) runs before the sub-dust
OP_RETURN validation and thus wrongly rejects valid OP_RETURN sub-dust outputs;
update the check so it only applies to non-sub-dust outputs by gating it with
the same sub-dust/OP_RETURN detection used later (reuse the existing sub-dust
detection/resolution logic referenced by resolveMinAmounts or the sub-dust
validation block), i.e., skip the s.vtxoMinAmount comparison when the output is
identified as an OP_RETURN sub-dust VTXO (preserve the existing
errors.AMOUNT_TOO_LOW behavior for all other outputs).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/core/application/service.gointernal/core/application/service_test.gointernal/core/application/utils.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/core/application/utils.go
- internal/core/application/service_test.go
bitcoin-coder-bob
force-pushed
the
bob/vtxoMinOffchainTxAmount
branch
from
2f3895c to
77e2422
Compare
internal/core/application/service.go
Outdated
| dustAmount, err := wallet.GetDustAmount(ctx) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to get dust amount: %s", err) | ||
| } | ||
| vtxoMinAmount, utxoMinAmount = resolveMinAmounts( | ||
| vtxoMinAmount, utxoMinAmount, int64(dustAmount), | ||
| ) |
There was a problem hiding this comment.
this must be dropped after merging #924. The changes made to Start() below are just enough
internal/core/application/service.go
Outdated
| } | ||
| } | ||
| if out.Value < s.vtxoMinOffchainTxAmount { | ||
| if out.Value < s.vtxoMinAmount && !script.IsSubDustScript(out.PkScript) { |
There was a problem hiding this comment.
| if out.Value < s.vtxoMinAmount && !script.IsSubDustScript(out.PkScript) { | |
| if out.Value < s.vtxoMinAmount { |
There was a problem hiding this comment.
@altafan #923 (review) do you disagree or agree with the AIs assessment?
Without this additional check sub-dust OP_RETURN outputs were rejected (at line:
arkd/internal/core/application/service.go
Line 979 in eb83ea9
ever reaching the OP_RETURN exemption (at line:
arkd/internal/core/application/service.go
Line 992 in eb83ea9
So should the flow be:
- skip the min-amount check for OP_RETURN scripts (i.e. add back in the
&& !script.IsSubDustScript(out.PkScript)) - enforce that sub-dust outputs must be OP_RETURN scripts
internal/core/application/service.go
Outdated
| } | ||
| } | ||
| if amount < uint64(s.vtxoMinSettlementAmount) { | ||
| if amount < uint64(s.vtxoMinAmount) { |
There was a problem hiding this comment.
In Start(), we should take care of storing the dust amount we get at line https://github.com/arkade-os/arkd/blob/master/internal/core/application/service.go#L227 and use it here
| if amount < uint64(s.vtxoMinAmount) { | |
| if amount < uint64(s.dustAmount) { |
There was a problem hiding this comment.
we want RegisterIntent to enforce the same minimum that GetInfo advertises: VtxoMinAmount: s.vtxoMinAmount
arkd/internal/core/application/service.go
Line 2162 in eb83ea9
SubmitOffchainTx enforces arkd/internal/core/application/service.go
Line 979 in eb83ea9
GetInfo would see one minimum but RegisterIntent would enforce a different one.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/core/application/service.go (1)
979-987:⚠️ Potential issue | 🟠 MajorExempt OP_RETURN sub-dust outputs before enforcing
vtxoMinAmount.Line 979 currently rejects sub-dust OP_RETURN outputs before the script-based exemption at Line 990 can run.
Suggested fix
- if out.Value < s.vtxoMinAmount { + if out.Value < s.vtxoMinAmount && !script.IsSubDustScript(out.PkScript) { return nil, errors.AMOUNT_TOO_LOW.New( "output #%d amount is lower than min vtxo amount: %d", outIndex, s.vtxoMinAmount, ).WithMetadata(errors.AmountTooLowMetadata{ OutputIndex: outIndex, Amount: int(out.Value), MinAmount: int(s.vtxoMinAmount), }) }🤖 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 979 - 987, The amount check currently rejects sub-dust OP_RETURN outputs before the script-based exemption runs; move or add the OP_RETURN-script exemption so it runs before the vtxoMinAmount check. Concretely, in the same function in service.go, detect OP_RETURN outputs using the existing script-exemption logic (the same check used around Line 990) for the current output (e.g., the output's script/pubkey variable) and skip the vtxoMinAmount validation for those outputs before evaluating "if out.Value < s.vtxoMinAmount" (so the errors.AMOUNT_TOO_LOW branch is not taken for OP_RETURN-exempt scripts). Ensure you preserve the WithMetadata payload and only bypass the amount check for true OP_RETURN-exempt scripts.
🤖 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 1883-1890: RegisterIntent enforces dust using s.dustAmount (amount
< s.dustAmount) but GetInfo currently returns s.vtxoMinAmount, creating a
mismatch; update GetInfo.VtxoMinAmount to report the same authoritative value
(s.dustAmount) or centralize both to a single field so they cannot diverge
(e.g., replace uses of s.vtxoMinAmount in GetInfo with s.dustAmount or alias
s.vtxoMinAmount = s.dustAmount), ensuring RegisterIntent, GetInfo and any other
callers reference the same symbol (s.dustAmount/GetInfo.VtxoMinAmount) to keep
advertised and enforced minima consistent.
---
Duplicate comments:
In `@internal/core/application/service.go`:
- Around line 979-987: The amount check currently rejects sub-dust OP_RETURN
outputs before the script-based exemption runs; move or add the OP_RETURN-script
exemption so it runs before the vtxoMinAmount check. Concretely, in the same
function in service.go, detect OP_RETURN outputs using the existing
script-exemption logic (the same check used around Line 990) for the current
output (e.g., the output's script/pubkey variable) and skip the vtxoMinAmount
validation for those outputs before evaluating "if out.Value < s.vtxoMinAmount"
(so the errors.AMOUNT_TOO_LOW branch is not taken for OP_RETURN-exempt scripts).
Ensure you preserve the WithMetadata payload and only bypass the amount check
for true OP_RETURN-exempt scripts.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/core/application/service.gointernal/core/application/service_test.gointernal/core/application/utils.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/core/application/service_test.go
3 participants
Issue: #846
I think we were just reporting back the wrong field?
Before:
After:
A client reads
vtxoMinAmountfromGetInfo, constructs aRegisterIntentwith an amount above that value, and gets rejected. The fix of reportingvtxoMinSettlementAmountdirectly solves this because that's the valueRegisterIntentchecks against.Move setting
vtxoMinSettlement,vtxoMinOffchain, andutxoMininto util function,resolveMinAmountsininternal/core/application/utils.goand added test for it ininternal/core/application/service_test.goin a test calledTestResolveMinAmount.Summary by CodeRabbit
Refactor
Tests