fix(reservations): address validated review findings on #4343

.github/workflows/client.yml

@@ -165,27 +165,34 @@ jobs:
     #   - the inKindFeeDebtSat and tbtcToken entries of the committed
     #     ReservationVault binding against the compiled ReservationVault
     #     artifact.
+    # It also checks the real validator that the in-memory EVM harness
+    # (pkg/chain/ethereum/tbtc_validator_harness_test.go) deploys:
+    # pkg/chain/ethereum/testdata/walletproposalvalidator/verify.sh
+    # compares the vendored WalletProposalValidator.json against the
+    # compiled artifact - the ABI exactly, and the creation and deployed
+    # bytecode with solc's trailing CBOR metadata removed (the metadata
+    # hashes the compiler input, which the unfrozen yarn install below
+    # can change without changing the code). An ABI-only diff cannot
+    # catch a validator whose logic changed under an identical ABI.
     # Cheap (checkout + yarn install + hardhat compile + a go-run ABI
     # extraction + jq diffs) and
     # runs whenever the Client workflow runs, without an additional
     # job-level path gate, mirroring btcec-vendor-byte-identity's
     # rationale for not gating by path.
     #
-    # tbtc-v2 ref: pinned to commit 9f8f5ef1 - the tip of threshold-network/tbtc-v2's
-    # `reservations-upgrade` integration branch (which merged
-    # threshold-network/tbtc-v2#1112, adding ReservationRouter.sol, into
-    # that branch, not `main`). A full commit hash is pinned rather than
-    # the branch name because a branch ref can move after this repo's
-    # generated bindings and vendored fragments are produced, and the
-    # diff above would then fail without any visible cause. `main` does
-    # not carry the reservation contracts yet (verified: no
-    # ReservationRouter.sol there as of this job's authoring), so
-    # pointing here at `main` would make every run of this job fail
-    # unconditionally. When the reservations work lands on tbtc-v2's
-    # `main`, switch `TBTC_V2_REF` back to `main`.
+    # tbtc-v2 ref: pinned to commit eec999aa, the merge of
+    # threshold-network/tbtc-v2#1161 into the `reservations-upgrade`
+    # integration branch, where the reservation contracts live (they are
+    # not on tbtc-v2's `main`). A full commit hash is pinned rather than
+    # a branch name because a branch can move after this repo's bindings,
+    # vendored fragments, and harness validator are produced, and the
+    # checks would then fail without any visible cause. The vendored
+    # harness validator is built from this same commit (see the
+    # provenance note in testdata/walletproposalvalidator/regenerate.sh),
+    # so bump both together.
     runs-on: ubuntu-latest
     env:
-      TBTC_V2_REF: 9f8f5ef1ca82f423571225066a291a6a0f963aac
+      TBTC_V2_REF: eec999aad43b3913df378840773348e17f68f1ba
     steps:
       - uses: actions/checkout@v4
 
@@ -254,6 +261,15 @@ jobs:
           cp "$BASE_VAULT/ReservationVault.sol/ReservationVault.json" "tmp/contracts/development/@keep-network/tbtc-v2/artifacts/ReservationVault.json"
           make -C pkg/chain/ethereum/tbtc/gen verify-vendored-fallback
 
+      - name: Verify the harness's vendored WalletProposalValidator matches the compiled validator
+        run: |
+          pkg/chain/ethereum/testdata/walletproposalvalidator/verify.sh \
+            tbtc-v2-upstream/solidity/build/contracts/bridge/WalletProposalValidator.sol/WalletProposalValidator.json \
+            || {
+              echo "::error::pkg/chain/ethereum/testdata/walletproposalvalidator/WalletProposalValidator.json does not match WalletProposalValidator compiled from tbtc-v2 ${TBTC_V2_REF} (see the output above). Regenerate it with regenerate.sh from that build, or align TBTC_V2_REF with its provenance note."
+              exit 1
+            }
+
   client-build-test-publish:
     needs: client-detect-changes
     if: |

cmd/maintainer.go

@@ -79,11 +79,9 @@ func maintainers(cmd *cobra.Command, args []string) error {
 
 	metricsRecorder := initializeMaintainerMetrics(ctx, blockCounter, tbtcChain, btcChain)
 
-	clientConfig.Maintainer.Spv.EthereumNetwork = clientConfig.Ethereum.Network
-
 	maintainer.Initialize(
 		ctx,
-		clientConfig.Maintainer,
+		maintainerConfig(clientConfig),
 		btcChain,
 		btcDiffChain,
 		tbtcChain,
@@ -94,6 +92,17 @@ func maintainers(cmd *cobra.Command, args []string) error {
 	return fmt.Errorf("unexpected context cancellation")
 }
 
+// maintainerConfig returns the maintainers' config with the SPV
+// maintainer's Ethereum network copied from the [ethereum] section. The
+// SPV maintainer looks up the reservation activation block by that
+// network; left unset, the network is Unknown and every reservation
+// catch-up scan is skipped.
+func maintainerConfig(cfg *config.Config) maintainer.Config {
+	maintainerConfig := cfg.Maintainer
+	maintainerConfig.Spv.EthereumNetwork = cfg.Ethereum.Network
+	return maintainerConfig
+}
+
 // initializeMaintainerMetrics sets up the client info registry and performance
 // metrics for the maintainer command. It returns a metrics recorder wired to
 // the SPV maintainer, or nil when the client info endpoint is not configured

cmd/maintainer_test.go

@@ -5,8 +5,13 @@ import (
 	"net"
 	"testing"
 
+	commonEthereum "github.com/keep-network/keep-common/pkg/chain/ethereum"
+
+	"github.com/keep-network/keep-core/config"
 	"github.com/keep-network/keep-core/pkg/bitcoin"
 	"github.com/keep-network/keep-core/pkg/clientinfo"
+	"github.com/keep-network/keep-core/pkg/maintainer"
+	"github.com/keep-network/keep-core/pkg/maintainer/spv"
 )
 
 // stubBlockCounter is a minimal chain.BlockCounter implementation used to
@@ -216,3 +221,30 @@ func TestInitializeMaintainerMetricsEnabledWhenPortSet(t *testing.T) {
 		)
 	}
 }
+
+// TestMaintainerConfig verifies that the maintainer command hands the SPV
+// maintainer the Ethereum network from the [ethereum] section: without it
+// the network is Unknown, the reservation activation block lookup fails
+// and every reservation catch-up scan is skipped. The rest of the
+// maintainer config passes through unchanged.
+func TestMaintainerConfig(t *testing.T) {
+	cfg := &config.Config{
+		Ethereum: commonEthereum.Config{Network: commonEthereum.Sepolia},
+		Maintainer: maintainer.Config{
+			Spv: spv.Config{Enabled: true, ReservationProofsEnabled: true},
+		},
+	}
+
+	got := maintainerConfig(cfg)
+
+	if got.Spv.EthereumNetwork != commonEthereum.Sepolia {
+		t.Errorf(
+			"expected the SPV maintainer network %v, got %v",
+			commonEthereum.Sepolia,
+			got.Spv.EthereumNetwork,
+		)
+	}
+	if !got.Spv.Enabled || !got.Spv.ReservationProofsEnabled {
+		t.Errorf("expected the rest of the SPV config to pass through, got %+v", got.Spv)
+	}
+}

pkg/chain/ethereum/tbtc.go

@@ -9,21 +9,9 @@
 // the per-concern files and reintroduce the old tbtc.go, silently dropping
 // whatever those later commits changed. Reconstructing the pre-split state
 // requires a manual merge, not a mechanical revert.
-// tbtc.go: TbtcChain adapter construction and shared state. See tbtc_*.go for
-// per-concern implementations (tbtc_deposit.go, tbtc_dkg.go, tbtc_moving_funds.go,
-// tbtc_redemption.go, tbtc_wallet.go, tbtc_sortition.go, tbtc_inactivity.go).
-//
-// These files were split out of a single monolithic tbtc.go with no rename
-// markers git can detect (each file is a fresh addition, not a tracked move),
-// so a plain `git revert` of the split commit cannot be applied cleanly on
-// top of any later commit that also touches this package: it would re-delete
-// the per-concern files and reintroduce the old tbtc.go, silently dropping
-// whatever those later commits changed. Reconstructing the pre-split state
-// requires a manual merge, not a mechanical revert.
 package ethereum
 
 import (
-	"context"
 	"crypto/ecdsa"
 	"encoding/binary"
 	"errors"
@@ -35,7 +23,6 @@ import (
 	"sync"
 	"time"
 
-	hostchain "github.com/ethereum/go-ethereum"
 	"github.com/ethereum/go-ethereum/common"
 	"github.com/ethereum/go-ethereum/common/hexutil"
 	"github.com/ethereum/go-ethereum/crypto"
@@ -677,6 +664,7 @@ func convertReservationFromAbiType(
 		RequestNonce:          abiReservation.RequestNonce,
 		RetryCredit:           abiReservation.RetryCredit,
 		DissolutionEligibleAt: abiReservation.DissolutionEligibleAt,
+		ReanchorCooldownUntil: abiReservation.ReanchorCooldownUntil,
 	}, nil
 }
 
@@ -689,6 +677,8 @@ func convertReservationFromAbiType(
 //   - `UsedRetryCredit`, `Watchtower{Default,LevelOne,LevelTwo}Delay`,
 //     `RetryCreditSourceNonce`: written for governance / late-settlement
 //     reconciliation but not read by the operator client in m1.
+//   - `DissolutionDelay`: snapshotted by acceptance generations for the
+//     dissolution path, which the operator client does not drive in m1.
 //
 // The on-chain `actionDataHash` field is polymorphic across action types:
 // it carries the keccak256 of the redeemer output script for redemptions,
@@ -843,81 +833,31 @@ func parseReservationActionState(value uint8) (tbtc.ReservationActionState, erro
 
 // RequestReservationReanchor asks the Bridge (via its ReservationRouter
 // delegatecall target) to start a new reservation re-anchor action generation
-// for the given reservation, targeting the given wallet. The returned
-// transaction hash lets the caller track the submission across coordination
-// rounds via GetReservationReanchorRequestReceipt; the submission launches
-// mining and gas bumping in the background and returns before the
-// transaction is mined.
+// for the given reservation, targeting the given wallet.
 func (tc *TbtcChain) RequestReservationReanchor(
 	reservationKey *big.Int,
 	targetWalletPublicKeyHash [20]byte,
-) ([32]byte, error) {
+) error {
 	gasEstimate, err := tc.reservationRouter.RequestReservationReanchorGasEstimate(
 		reservationKey,
 		targetWalletPublicKeyHash,
 	)
 	if err != nil {
-		return [32]byte{}, err
+		return err
 	}
 
 	// Here we add a 20% margin to overcome the gas problems.
 	gasEstimateWithMargin := float64(gasEstimate) * float64(1.2)
 
-	tx, err := tc.reservationRouter.RequestReservationReanchor(
+	_, err = tc.reservationRouter.RequestReservationReanchor(
 		reservationKey,
 		targetWalletPublicKeyHash,
 		ethutil.TransactionOptions{
 			GasLimit: uint64(gasEstimateWithMargin),
 		},
 	)
-	if err != nil {
-		return [32]byte{}, err
-	}
-
-	return [32]byte(tx.Hash().Bytes()), nil
-}
-
-// GetReservationReanchorRequestReceipt reports the mining status of a
-// previously submitted RequestReservationReanchor transaction, as defined
-// by the tbtc.ReservationReanchorRequestReceiptStatus values. The receipt
-// lookup is bounded by a 30-second deadline, matching the baseChain header
-// helpers.
-//
-// A receipt that does not exist yet - which go-ethereum reports as
-// ethereum.NotFound for unknown or unmined hashes - maps to NotFound: the
-// caller treats NotFound and Pending identically, bounded by the block the
-// submission happened in. Every other lookup error (RPC outage, timeout,
-// transport failure) is returned to the caller so an in-flight request is
-// not mistaken for a dropped one.
-func (tc *TbtcChain) GetReservationReanchorRequestReceipt(
-	txHash [32]byte,
-) (tbtc.ReservationReanchorRequestReceiptStatus, error) {
-	ctx, cancelCtx := context.WithTimeout(
-		context.Background(),
-		30*time.Second,
-	)
-	defer cancelCtx()
 
-	receipt, err := tc.baseChain.client.TransactionReceipt(
-		ctx,
-		common.BytesToHash(txHash[:]),
-	)
-	if err != nil {
-		if errors.Is(err, hostchain.NotFound) {
-			return tbtc.ReservationReanchorRequestReceiptNotFound, nil
-		}
-		return tbtc.ReservationReanchorRequestReceiptNotFound, fmt.Errorf(
-			"cannot fetch transaction receipt: %w",
-			err,
-		)
-	}
-	if receipt == nil {
-		return tbtc.ReservationReanchorRequestReceiptNotFound, nil
-	}
-	if receipt.Status == 0 {
-		return tbtc.ReservationReanchorRequestReceiptReverted, nil
-	}
-	return tbtc.ReservationReanchorRequestReceiptMined, nil
+	return err
 }
 
 // SubmitReservationAcceptanceProof submits an SPV proof for the given
@@ -1681,18 +1621,19 @@ func (tc *TbtcChain) ActiveReservationsCount() (uint32, uint32, error) {
 	return activeReservationsCount.Count, activeReservationsCount.MaxActive, nil
 }
 
-// reservationVaultBindings returns the cached, per-vault-address bindings
-// (the ReservationVault itself and the TBTC token it holds its fee
-// reserve in) constructed against a vault address read from the Bridge's
-// on-chain reservation parameters. Successful constructions are cached per
-// vault address. Failures are not cached: they are usually transient RPC
-// errors (for example reading tbtcToken()), and caching them would disable
-// the fee gauges until the process restarts.
+// reservationVaultBindings holds the ReservationVault contract and the TBTC
+// token contract the vault keeps its fee reserve in.
 type reservationVaultBindings struct {
 	vault *tbtccontract.ReservationVault
 	tbtc  *tbtccontract.TBTC
 }
 
+// reservationVaultBindings returns the cached, per-vault-address bindings
+// (the ReservationVault itself and the TBTC token it holds its fee
+// reserve in) for the given vault address. Successful constructions are
+// cached per vault address. Failures are not cached: they are usually
+// transient RPC errors (for example reading tbtcToken()), and caching them
+// would disable the fee gauges until the process restarts.
 func (tc *TbtcChain) reservationVaultBindings(vaultAddress common.Address) (*reservationVaultBindings, error) {
 	if bindings, ok := tc.vaultBindings.Load(vaultAddress); ok {
 		return bindings.(*reservationVaultBindings), nil
@@ -1743,21 +1684,16 @@ func (tc *TbtcChain) reservationVaultBindings(vaultAddress common.Address) (*res
 	return bindings, nil
 }
 
-// reservationVaultAddress resolves the on-chain reservation vault address
+// decodeReservationVaultAddress decodes a reservation vault address read
 // from the Bridge's reservation parameters. It returns the zero address
 // when the vault is not configured (zero address in the parameters) and an
-// error only when the configured address cannot be decoded as a valid
-// 20-byte address, so callers can distinguish "no vault" (a skip, not
-// an error) from a genuinely malformed configuration.
-func (tc *TbtcChain) reservationVaultAddress() (common.Address, error) {
-	reservationParameters, err := tc.ReservationParameters()
-	if err != nil {
-		return common.Address{}, fmt.Errorf(
-			"cannot get reservation parameters: [%v]",
-			err,
-		)
-	}
-	vaultAddressString := string(reservationParameters.ReservationVault)
+// error only when the address cannot be decoded as a valid 20-byte
+// address, so callers can distinguish "no vault" (a skip, not an error)
+// from a genuinely malformed configuration.
+func decodeReservationVaultAddress(
+	reservationVault chain.Address,
+) (common.Address, error) {
+	vaultAddressString := string(reservationVault)
 	vaultAddressBytes, err := hexutil.Decode(vaultAddressString)
 	if err != nil || len(vaultAddressBytes) != common.AddressLength {
 		if err == nil {
@@ -1775,14 +1711,16 @@ func (tc *TbtcChain) reservationVaultAddress() (common.Address, error) {
 	return common.BytesToAddress(vaultAddressBytes), nil
 }
 
-// ReservationVaultFeeDebtSat returns the ReservationVault's outstanding
-// in-kind fee debt in satoshi, read from the vault's inKindFeeDebtSat
-// view. The vault address is resolved from the on-chain reservation
-// parameters; when it is the zero address the vault is not configured
-// and the method returns 0 with a nil error: the skip sentinel the
-// metrics side consumes, not a chain error.
-func (tc *TbtcChain) ReservationVaultFeeDebtSat() (uint64, error) {
-	vaultAddress, err := tc.reservationVaultAddress()
+// ReservationVaultFeeDebtSat returns the given ReservationVault's
+// outstanding in-kind fee debt in satoshi, read from the vault's
+// inKindFeeDebtSat view. The caller passes the vault address it already
+// read from the reservation parameters; when it is the zero address the
+// vault is not configured and the method returns 0 with a nil error: the
+// skip sentinel the metrics side consumes, not a chain error.
+func (tc *TbtcChain) ReservationVaultFeeDebtSat(
+	reservationVault chain.Address,
+) (uint64, error) {
+	vaultAddress, err := decodeReservationVaultAddress(reservationVault)
 	if err != nil {
 		return 0, err
 	}
@@ -1800,14 +1738,16 @@ func (tc *TbtcChain) ReservationVaultFeeDebtSat() (uint64, error) {
 	return debtSat, nil
 }
 
-// ReservationVaultFeeReserveTbtcBaseUnits returns the ReservationVault's
-// TBTC fee-reserve balance in TBTC base units (whole TBTC x 1e18). The
-// value is a token balance of a 1e18-scale token, which can exceed
-// uint64 range, so it is returned as *big.Int. When the vault is not
-// configured (zero address) the method returns zero with a nil error,
-// mirroring ReservationVaultFeeDebtSat's skip sentinel.
-func (tc *TbtcChain) ReservationVaultFeeReserveTbtcBaseUnits() (*big.Int, error) {
-	vaultAddress, err := tc.reservationVaultAddress()
+// ReservationVaultFeeReserveTbtcBaseUnits returns the given
+// ReservationVault's TBTC fee-reserve balance in TBTC base units (whole
+// TBTC x 1e18). The value is a token balance of a 1e18-scale token, which
+// can exceed uint64 range, so it is returned as *big.Int. When the vault
+// is not configured (zero address) the method returns zero with a nil
+// error, mirroring ReservationVaultFeeDebtSat's skip sentinel.
+func (tc *TbtcChain) ReservationVaultFeeReserveTbtcBaseUnits(
+	reservationVault chain.Address,
+) (*big.Int, error) {
+	vaultAddress, err := decodeReservationVaultAddress(reservationVault)
 	if err != nil {
 		return new(big.Int), err
 	}