[Version-unspecific] fix(bifrost/btc,dash): THORNode parity for the BTC and DASH clients

Bitol Mayarequested to merge
fix/bifrost-btc-dash-thor-parity into develop

Summary

This ports THORNode UTXO-client fixes to MAYANode's own BTC and DASH bifrost clients (bifrost/pkg/chainclients/bitcoin, bifrost/pkg/chainclients/dash). They come from the BCH port audit (findings BC-3, BC-5, BC-7, H-12, H-14, H-15, H-16), which found BTC and DASH missing them. Only bifrost changes; no consensus change. Commits 1-6 are the fixes; commits 7-9 close the gaps a later check against each THOR MR found (see "THORNode references and gap check").

  1. Stale asgard cache on fetch errors (BC-3, BC-7; thornode!4594, e91742a061)

    • Problem: when MAYANode could not be read, getAsgardAddresses dropped the cached list. Every tx in the block then looked unrelated, and the scanner moved past the block.
    • Fix: a shared helper, utxo.GetAsgardAddressesCached, returns the last known addresses together with the error, and isAsgardAddress keeps matching against them. FetchTxs retries the block (ErrRetryableBlockScan) when nothing is cached yet.
    • BTC confirmation count: this used to require 0 confirmations on an asgard error. It now fails closed: vault spends count as inbound value, which only raises the requirement.

  2. RPC deadlines (H-15; thornode!4596, 8d219c074)

    • Problem: the btcd/dashd-go rpcclient has no timeout and sends requests one at a time from a single goroutine. One hung daemon call blocks every later call, including the ones the signer makes while it holds the vault lock.
    • Fix: utxo.SetRPCClientTimeout sets http.Client.Timeout on the client's unexported httpClient field (block_scanner.http_request_timeout, 30s, falling back to 2 min). It returns an error, rather than doing nothing, if a dependency update renames that field.
    • DASH: the dashd-go client retries a failed request up to 10 times, so a DASH call to a daemon that never answers is bounded at about 10 × timeout + 27s.

  3. Mempool chain limit (H-14; thornode!4287 f79a1b781, thornode!4392 753d1c1d84)

    • Problem: selection could pick unconfirmed self outputs whose mempool chain was already at the daemon's limit, and the broadcast then failed with -26 too-long-mempool-chain.
    • Fix: such outputs are skipped when ancestorcount + descendantcount >= 25. 25 is the bitcoind default, which dashd inherits. If the mempool entry cannot be read, the output stays eligible.

  4. Satoshi sums (H-12; thornode!3348 44cefa21d, thornode!4350 66cdfdf7c)

    • Problem: selection, consolidation and GetAccount summed float64 RPC amounts. The sum can be one satoshi off the outputs. For consolidation that means "not enough balance", or a 1-sat change output.
    • Fix: every amount is converted to satoshis before it is added.

  5. BTC network fee cap (H-16; MAYA-only, no THORNode MR)

    • Problem: the BTC client posts a block's average fee rate as the network fee, with no upper bound.
    • Fix: the posted rate is now capped at 9765 sat/vB. That is the rate buildTx already caps a signed tx's fee at (0.1 BTC/kvB), the same number as THORNode's BTC max_sats_per_vbyte. THORNode itself applies that limit only when signing and posts the rate uncapped, so this cap is MAYA's own hardening. A miner can no longer set users' outbound fee above what the signer would ever pay. The min-relay floor is unchanged. DASH posts a fixed rate, so it needs no cap.

  6. Keysign party never formed (BC-5; thornode!5136, c69669e840)

    • Problem: vault members that built different unsigned txs each retried their own checkpoint forever, so no keysign party formed until MAYAChain rescheduled the outbound.
    • Fix: on a first attempt where every input failed with a join-party failure (TssSyncFail, no round), no checkpoint is kept. The next attempt rebuilds the tx. tss.KeysignError.IsJoinPartyFailure is the same code as the BCH branch's.
    • Panic fixed: the BTC/DASH TSS signable sliced the TSS result without checking its length, so an empty or short signature would have panicked inside SignTx's signing goroutine and crashed bifrost. MAYA's KeySign does not return one today, so the check is defensive. Commit 6 made it an error; commit 7 classifies an empty result as tss.ErrNotSelectedForKeysign.

  7. Not selected for keysign (gap in 6; thornode!5136, THORNode 5f2141c33e)

    • Problem: THORNode's KeySign maps an empty go-tss response to an empty signature, and its SignTx then stands down without a checkpoint. MAYA's KeySign does not: a node outside the committee gets the committee's signature, or a KeysignError for the empty response (Status 0). In MAYA this path is defensive and is not reached through today's KeySign.
    • Fix: an empty TSS result is tss.ErrNotSelectedForKeysign. When every input failed that way on a first attempt (no checkpoint yet), SignTx returns no tx, no checkpoint and no error, and posts no blame (as THORNode). A retry from a checkpoint keeps the checkpoint and returns an error, because an earlier attempt may have produced signature shares (commit 10; stricter than THORNode). remoteSign is unchanged, so other chains behave as before. Unlike THORNode, a node that signed some inputs keeps its checkpoint, because it may have exposed a signature share.

  8. Consolidation flag and UTXO listing error (gaps in 1 and 4; thornode!4594, thornode!4350)

    • consolidateInProgress is claimed with CompareAndSwap(false, true) instead of Load then Store.
    • buildTx wraps the cause: fail to get unspent UTXO: %w.

  9. Insufficient UTXOs (gap in 3; thornode!4287 f79a1b781, follow-up eaf7480a37)

    • Problem: getUtxoToSpend returned a short set, and buildTx failed later with a confusing "not enough balance".
    • Fix: it returns "insufficient available UTXOs: need X, only have Y ..." when the selection falls short. A floor (the outbound amount) still lets a vault with less than amount + MaxGas pay amount + the actual fee, as THORNode eaf7480a37. The consolidation probe asks for 0 instead of 0.01 coin, so a vault of dust still consolidates.

THORNode references and gap check

Every change in the THOR MRs below was compared with this MR.

Fix THORNode MR Result
1 stale asgard cache !4594 Ported; the CAS gap is fixed in 8. THOR's solvency lock, fee lock and fee-rate cache do not apply (MAYA BTC/DASH have none). The ETH/ZEC part is !882 (merged).
2 RPC deadlines !4596 Adapted: http.Client.Timeout instead of per-call contexts. The vault lock held through broadcast was already in MAYA.
3 mempool chain limit !4287, !4392 Ported, with a constant 25 instead of THOR's config. The "insufficient UTXOs" gap is fixed in 9.
4 satoshi sums !3348, !4350 Ported; the error-wrap gap is fixed in 8.
5 BTC fee cap none MAYA-only (see above).
6 party never formed !5136 Ported; the not-selected path is added in 7 (defensive in MAYA) and limited to attempts without a checkpoint in 10.

Behaviour changes reviewers should check

  • Keysign blame (commit 6): signUTXO no longer posts keysign blame once per input. It used to overwrite the KeysignError with the post result, which dropped it from the error chain. Blame is now posted once per SignTx by utxo.PostKeysignFailure, as in THORNode.
  • Round7Retry (commit 6): as a side effect, bifrost/signer now recognises terminal-round (KEYSIGN9) failures that carry blame nodes and sets Round7Retry. Before, it did so only when the blame named no node.
  • Posted BTC fee (commit 5): during a mixed rollout, nodes with the cap and nodes without it post different values only when a block's average fee rate exceeds 9765 sat/vB.
  • Not selected (commits 7, 10): if TSS ever returns an empty signature with no error on a first attempt, the node drops the item from its local signer storage, as THORNode does, and MAYAChain reschedules the outbound if the committee fails. Through MAYA's KeySign today, a node outside the committee instead gets a KeysignError and keeps its checkpoint, as before this MR. A retry from a checkpoint never drops it.
  • UTXO selection (commit 9): an outbound whose eligible UTXOs do not cover the amount now fails at selection with "insufficient available UTXOs" instead of later in buildTx. Either way no tx is signed.
  • Scanner retry (commit 1): FetchTxs now returns a retryable error (block retried, scanner marked unhealthy) when MAYANode is unreachable and no asgard list was ever loaded, typically right after a bifrost restart.

Not in this MR

  • Signer-cache expiry (THORNode a759cb4f99, BC-4): !880 (merged).
  • The ETH and ZEC part of thornode!4594: !882 (merged).
  • Keysign height pinning (BC-6), vout tie-break (H-13), and the THORNode 2 sat/vB fee floor: not requested. The fee floor changes outbound fees and is a separate decision.

Tests

  • New unit tests: each fix has unit tests in the bitcoin, dash, shared/utxo and tss packages. The behaviour tests fail on develop; the BTC fee-cap test does not compile there (networkFeeRate is new). TestSignTxEmptyKeysignResponseKeepsCheckpoint documents current KeySign behaviour and passes without the fix.

  • Commands run:

    go build ./bifrost/...
go vet ./bifrost/tss/ ./bifrost/pkg/chainclients/{bitcoin,dash,shared/utxo}/
go test -p 2 -count=1 ./bifrost/pkg/chainclients/bitcoin/... ./bifrost/pkg/chainclients/dash/... \
    ./bifrost/pkg/chainclients/shared/utxo/... ./bifrost/tss/

    All pass.

  • bifrost/signer: passes with -tags mocknet. Without the tag, 9 tests fail (Storage/Sign/ExpirySigner suites); the same 9 fail on unmodified develop cf9714438.

  • Not run: regression, smoke, and make lint/trunk.

Review follow-up

  • ea707aaba (commit 10): the not-selected stand-down applies only when no checkpoint exists, in BTC and DASH. A retry from a checkpoint keeps it and returns an error. The comments now say the empty-signature path is defensive: MAYA's KeySign gives a node outside the committee the committee's signature or a KeysignError. The commit message of commit 7 states the earlier premise; this section supersedes it. Tests: TestSignTxNotSelectedRetryKeepsCheckpoint (fails without the guard) and TestSignTxEmptyKeysignResponseKeepsCheckpoint (real KeySign adapter), both in BTC and DASH.
  • b71215f8d: GetAsgardAddressesCached remembers a failed refresh for AsgardRefreshBackoff (2s) and returns the stale addresses with the error, without querying MAYANode again. This is the same change as !882 (merged)'s review follow-up, so the shared helper stays identical in both MRs.
  • 12a392eb6: BTC and DASH FetchTxs fail the block on any asgard refresh error, also with a stale list cached, so a deposit to a vault that became active since the last refresh is retried instead of skipped (the gap found on !882 (merged)). The DASH suite shares one client, so SetUpTest clears the cache. Tests: TestFetchTxsRetriesWithoutAsgardAddresses (BTC and DASH) and TestFetchTxsRetriesDepositToNewVault (BTC).
Edited by Bitol Maya

Merge request reports