Refund SC deposits destroyed by an incomplete call - #74
Conversation
A burn attached to an SC call was debited and then destroyed on every path that did not reach the refund block: three early returns inside the action switch, two exits above it, an SC_INSTALL carrying no code that nil-dereffed into the swallowed recover and returned success, and duplicate-SCID payloads whose burns were overwritten rather than summed. These now funnel into the existing refund path through ErrorRevertHF3, a non-panicking ErrorRevert. ErrorRevert and ErrorDeposit are unchanged, so pre-fork replay is byte-identical by inspection. Accumulating the payload burns also corrects the success path, where a burn on any payload but the last was debited from the sender and credited to nobody. Gated on config.BLACKHOLE_HEIGHT: tied relatively to MAJOR_HF3_HEIGHT on mainnet, and set above the head on testnet, where MAJOR_HF3_HEIGHT is 0 and a retro-active state rule diverges the balance tree, fails a later roothash check and wedges resync permanently. A deposit above ringsize 2 has no recoverable signer, since Extract_signer reads one only from a zero-SCID ringsize-2 payload, so it cannot be refunded in any era. walletapi refuses to build that shape at a defaulted ring size; an explicit ringsize is honoured and warned, without recording the burn value or the scid. It is deliberately not refused at verification: that verifier also runs at block-add, and the stock wallet's default ringsize 16 produces the shape, so a refusal there would reject blocks produced by un-upgraded miners. cmd/simulator/blackhole_test.go runs both eras over eight tx shapes; the pre-fork mode reproduces the unpatched baseline exactly. walletapi/tx_sc_deposit_guard_test.go pins the build-time guard without a chain and reads the wire-form ringsize, so a refusal cannot be swapped for a silent downgrade. Gate CI on -run Test_SCDeposit_Guard; the walletapi package is red at baseline for unrelated reasons.
The chain-level test only ever burns plain DERO, so the two branches that credit token assets back to the signer were never executed. This drives all three switch arms directly against graviton trees and asserts the exact credited balance, plus a negative control for a signer that holds no account in the asset's tree. Both token branches were mutation-proven: disabling each one fails this test.
8lecramm
left a comment
8lecramm
left a comment
There was a problem hiding this comment.
What about a pre-check before adding the TX to the mempool?
There are limitations: the precheck uses current state, not the state at the time the TX will actually be mined, so SCs whose state changes between now and mining can still fail at execution time. But it catches the most common cases: non-existent SCIDs, bad entrypoints, and statically-invalid calls.
|
Yes. It catches more than you listed: I ran GetGasEstimate against mainnet and a call the contract can't fund comes back as a clean error too. Your drift caveat is real and I don't have a way around it. First though, BLACKHOLE_HEIGHT is tied to MAJOR_HF3_HEIGHT, now ~64,000 blocks behind us, so the rule would apply retro-actively. Where do you want it? Needs a version bump too. |
|
Different question, and it's yours to call. With no signer to refund, should the deposit go to the contract's own balance, get burned, or should the tx just be refused? ErrorDeposit does the first and has no callers. Your pre-check is close to the third. |
2 participants
Refund SC deposits destroyed by an incomplete call
WHAT HAPPENS TODAY
DERO attached to a smart-contract call is debited from the sender and then
destroyed if the call does not complete. The transaction mines, the sender is
charged, and the deposit is gone. Nothing is credited to the contract and
nothing returns to the sender.
The shapes that lose funds:
Only the sender's own funds are affected. There is no path here for one
account to destroy another's balance, and no inflation: the amount is burned,
not moved. That is why this is being raised in the open rather than privately.
WHAT THIS CHANGES
Post-activation, a call that does not complete refunds the deposit to the
sender instead of burning it. Every behaviour change is gated on
BLACKHOLE_HEIGHT and lives in process_transaction_sc. Blocks below the
activation height execute exactly as they do now, so a patched and an
unpatched node agree on all existing history.
On mainnet BLACKHOLE_HEIGHT is tied to MAJOR_HF3_HEIGHT so the rule activates
with the fork.
On testnet it is deliberately NOT tied to it. MAJOR_HF3_HEIGHT is 0 there, so
tying them would make the refund rule retro-active to testnet genesis, and a
patched node replaying testnet history would diverge at the first historical
transaction that fails in one of the newly-routed ways. The testnet value is a
placeholder set above any plausible current head and should be tightened at
release against the live testnet head.
THE WALLET-SIDE LIMITATION
A refund needs a sender to refund to. At ringsize 2 the payload exposes a
recoverable signer; above ringsize 2 it does not, by construction. So the
refund cannot be made universal on chain without changing what a ring proof
means.
Rather than let the wallet build a transaction whose deposit cannot be
recovered, walletapi refuses that shape and explains why.
To be explicit about the tradeoff, because the fix should not become a nudge
in the wrong direction: ringsize 2 identifies you as the sender. This is a
limitation being documented, not a practice being recommended. Nobody should
lower their ring size for refundability as a matter of course. The honest
summary is that deposits are recoverable only in a shape that costs sender
anonymity, and that above ring 2 the deposit remains at risk if the call does
not complete.
One further consequence worth flagging: ringsize 2 also lowers the wallet's
auto-computed fee, and that fee is the storage-gas budget. A call that stores
near the limit can therefore fail for insufficient storage gas after dropping
to ring 2. Pass an explicit fee if the call writes more than a few hundred
bytes.
TESTING
go test ./walletapi/ -run Test_SCDeposit_Guard
go test ./cmd/simulator/ -run Test_Blackhole_SC_Deposit_Loss
The simulator test runs both sides of the activation height in one pass: it
asserts the loss still occurs below it and the refund occurs above it, and
carries a control shape that refunds in both eras. The wallet guard test
covers both the defaulted-ringsize refusal and the explicit-ringsize warning.
Verified against this branch's base: applied to a clean checkout of Release151
(898730e), go build ./... is clean and both suites pass.
WHAT THIS DOES NOT DO
Ringsize above 2 is not fixed by this patch, for the reason above. That
residual is deliberate and documented in the code rather than papered over.
The specific mainnet transactions that lost funds are matched by shape, not by
reproduced mechanism. I have not reproduced the exact cause of any individual
mainnet loss, and this patch should not be read as claiming that.
NOTES FOR REVIEW
Two designs were tried and rejected before this one, both worth knowing about
if you are considering alternatives:
Returning an error for the no-signer case halts the chain. It poisons
sc_change_cache and block-commit then panics outside the recover. Reproduced
twice.
Refusing the shape at verification time partitions the network, because
block-add rejects whole blocks and the stock wallet's default ringsize
produces the refused shape.
Every node must be upgraded before the activation height, as with any
height-gated state rule.
Happy to rebase, re-gate behind a different constant, or split the wallet
guard into its own PR if you would rather take the consensus change alone.