diff --git a/.github/workflows/_shared-check.yaml b/.github/workflows/_shared-check.yaml index 72a50a5b..5ad7f78d 100644 --- a/.github/workflows/_shared-check.yaml +++ b/.github/workflows/_shared-check.yaml @@ -30,7 +30,7 @@ jobs: run: if [ "$(gofmt -s -l . | wc -l)" -gt 0 ]; then exit 1; fi - name: Install staticcheck - run: go install honnef.co/go/tools/cmd/staticcheck@latest + run: go install honnef.co/go/tools/cmd/staticcheck@v0.7.0 - name: Run staticcheck run: staticcheck ./... diff --git a/docs/02-global-config.md b/docs/02-global-config.md index 57a8859f..0d28895c 100644 --- a/docs/02-global-config.md +++ b/docs/02-global-config.md @@ -25,6 +25,20 @@ endpoints: - name: "node-1" executionUrl: "http://127.0.0.1:8545" consensusUrl: "http://127.0.0.1:5052" + # executionGroups puts an endpoint in spamoor client groups, so a scenario + # can direct its transactions at it with its client_group option. + # + # Membership is ADDITIVE: every endpoint is in "default", which is also the + # group used by every selection that names none - wallet funding and + # refills, contract deployments, and scenarios without client_group. Naming + # a group does not stop that traffic. Add "-default" to reserve an endpoint + # for the scenarios that ask for it, e.g. a builder's private tx intake that + # must hold only one scenario's transactions. Removal needs a spamoor + # version that supports it; assertoor fails at startup if it is ignored. + - name: "buildoor-intake" + executionUrl: "http://127.0.0.1:8085/rpc" + consensusUrl: "http://127.0.0.1:5052" + executionGroups: ["builder", "-default"] validatorNames: inventoryYaml: "./validator-names.yaml" diff --git a/pkg/clients/clients.go b/pkg/clients/clients.go index ab565b39..926fc247 100644 --- a/pkg/clients/clients.go +++ b/pkg/clients/clients.go @@ -40,6 +40,21 @@ type ClientConfig struct { ConsensusHeaders map[string]string `yaml:"consensusHeaders"` ExecutionURL string `yaml:"executionUrl"` ExecutionHeaders map[string]string `yaml:"executionHeaders"` + // ExecutionGroups are the spamoor client groups this execution endpoint + // belongs to, so a scenario can direct its transactions at specific + // endpoints via its client_group option. + // + // Membership is ADDITIVE: every client is in "default", which is also the + // group used by every selection that names none (wallet funding and + // refills, contract deployments, scenarios without client_group). Naming a + // group therefore does not stop that traffic. To reserve an endpoint — + // e.g. a builder's private tx intake that must hold only one scenario's + // transactions — also remove it from the default group with "-default". + // Removal requires a spamoor version that supports it; assertoor fails at + // startup if the linked one ignores the entry. + // + // executionGroups: ["builder", "-default"] + ExecutionGroups []string `yaml:"executionGroups"` } func NewClientPool(logger logrus.FieldLogger) (*ClientPool, error) { @@ -95,6 +110,7 @@ func (pool *ClientPool) AddClient(config *ClientConfig) error { Name: config.Name, URL: config.ExecutionURL, Headers: config.ExecutionHeaders, + Groups: config.ExecutionGroups, }) if err != nil { return fmt.Errorf("could not init consensus client: %w", err) diff --git a/pkg/clients/execution/client.go b/pkg/clients/execution/client.go index 3114d36e..d5a1be9e 100644 --- a/pkg/clients/execution/client.go +++ b/pkg/clients/execution/client.go @@ -22,6 +22,7 @@ type ClientConfig struct { URL string Name string Headers map[string]string + Groups []string // spamoor client groups; empty = default } type Client struct { diff --git a/pkg/txmgr/spamoor.go b/pkg/txmgr/spamoor.go index e411b1d2..f9c87f9c 100644 --- a/pkg/txmgr/spamoor.go +++ b/pkg/txmgr/spamoor.go @@ -45,7 +45,23 @@ func NewSpamoor(ctx context.Context, logger logrus.FieldLogger, executionPool *e return nil, err } - for i, client := range clientPool.GetAllClients() { + // InitClients skips options it could not turn into a client, so a short + // list would silently pair every later endpoint with another endpoint's + // client — wiring transactions to the wrong node and checking the wrong + // endpoint's groups. A client only fails to build on a malformed rpchost, + // which is a config error worth stopping for. + spamoorClients := clientPool.GetAllClients() + if len(spamoorClients) != len(endpoints) { + return nil, fmt.Errorf("spamoor initialized %d of %d execution endpoints; "+ + "check the endpoint configuration (executionGroups, headers)", + len(spamoorClients), len(endpoints)) + } + + for i, client := range spamoorClients { + if groupErr := verifyClientGroups(endpoints[i].GetEndpointConfig(), client); groupErr != nil { + return nil, groupErr + } + s.clients[endpoints[i]] = client } @@ -94,6 +110,10 @@ func (s *Spamoor) getClientOptions(client *execution.Client) *spamoor.ClientOpti rpcURL := client.GetEndpointConfig().URL rpcURL = fmt.Sprintf("name(%s)%s", client.GetName(), rpcURL) + if groups := normalizedGroups(client.GetEndpointConfig()); len(groups) > 0 { + rpcURL = fmt.Sprintf("group(%s)%s", strings.Join(groups, ","), rpcURL) + } + if headers := client.GetEndpointConfig().Headers; len(headers) > 0 { headerParts := make([]string, 0, len(headers)) for key, value := range headers { @@ -269,3 +289,49 @@ func (s *Spamoor) NewWalletPoolByPrivkey(ctx context.Context, logger logrus.Fiel return walletPool, nil } + +// verifyClientGroups checks that spamoor applied the endpoint's configured +// client groups. Group membership decides which scenarios reach an endpoint, +// so a silently ignored entry would send transactions somewhere the operator +// deliberately excluded. A "-name" entry (removing a group, e.g. "-default" +// to keep an endpoint out of the group-less selections) needs a spamoor +// version that supports removal; older versions treat it as a literal group +// name, which this check turns into a startup error instead of a surprise. +func verifyClientGroups(config *execution.ClientConfig, client *spamoor.Client) error { + for _, group := range normalizedGroups(config) { + if remove, found := strings.CutPrefix(group, "-"); found { + if client.HasGroup(remove) { + return fmt.Errorf( + "endpoint %q: executionGroups asked to remove client group %q but the client is still in it "+ + "(the linked spamoor does not support group removal)", + config.Name, remove) + } + + continue + } + + if !client.HasGroup(group) { + return fmt.Errorf("endpoint %q: executionGroups requested client group %q but the client is not in it", + config.Name, group) + } + } + + return nil +} + +// normalizedGroups trims the endpoint's configured client groups and drops +// empty entries, matching what spamoor's own group(...) parser does. Without +// it a stray space makes the two disagree: spamoor applies the trimmed name +// while the check compares the raw one, which both fails startup spuriously +// on an added group and, worse, passes a removal that never took effect. +func normalizedGroups(config *execution.ClientConfig) []string { + groups := make([]string, 0, len(config.Groups)) + + for _, group := range config.Groups { + if group = strings.TrimSpace(group); group != "" { + groups = append(groups, group) + } + } + + return groups +} diff --git a/pkg/txmgr/spamoor_groups_test.go b/pkg/txmgr/spamoor_groups_test.go new file mode 100644 index 00000000..dc2a6398 --- /dev/null +++ b/pkg/txmgr/spamoor_groups_test.go @@ -0,0 +1,64 @@ +package txmgr + +import ( + "slices" + "testing" + + "github.com/ethpandaops/assertoor/pkg/clients/execution" + "github.com/ethpandaops/spamoor/spamoor" +) + +// spamoor trims group names in its own parser, so the check must trim too: +// otherwise a stray space makes the two disagree and a removal that never +// took effect passes verification — the silent under-isolation this guard +// exists to prevent. +func TestNormalizedGroupsMatchesSpamoorParsing(t *testing.T) { + config := &execution.ClientConfig{ + Name: "intake", + URL: "http://localhost:8545", + Groups: []string{" builder ", "", " ", "-default "}, + } + + got := normalizedGroups(config) + if want := []string{"builder", "-default"}; !slices.Equal(got, want) { + t.Fatalf("normalizedGroups = %v, want %v", got, want) + } + + client, err := spamoor.NewClient(&spamoor.ClientOptions{ + RpcHost: "group(" + got[0] + ")" + config.URL, + }) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if !client.HasGroup("builder") { + t.Error("spamoor did not apply the trimmed group name") + } + + if err := verifyClientGroups(&execution.ClientConfig{Name: "intake", Groups: []string{" builder "}}, client); err != nil { + t.Errorf("verification must accept the same trimmed name spamoor applied: %v", err) + } +} + +// A configured group the linked spamoor did not apply must stop startup. +func TestVerifyClientGroupsRejectsAnUnappliedEntry(t *testing.T) { + client, err := spamoor.NewClient(&spamoor.ClientOptions{RpcHost: "group(builder)http://localhost:8545"}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if applied := verifyClientGroups(&execution.ClientConfig{Name: "intake", Groups: []string{"builder"}}, client); applied != nil { + t.Errorf("an applied group must verify: %v", applied) + } + + // The pinned spamoor keeps the client in "default", so a removal that was + // ignored has to be an error rather than silent under-isolation. + ignored := verifyClientGroups(&execution.ClientConfig{Name: "intake", Groups: []string{"-default"}}, client) + if client.HasGroup("default") && ignored == nil { + t.Error("an ignored removal must fail verification") + } + + if err := verifyClientGroups(&execution.ClientConfig{Name: "intake", Groups: []string{"absent"}}, client); err == nil { + t.Error("a group the client is not in must fail verification") + } +} diff --git a/playbooks/dev/buildoor-testing-build.yaml b/playbooks/dev/buildoor-testing-build.yaml new file mode 100644 index 00000000..3606165b --- /dev/null +++ b/playbooks/dev/buildoor-testing-build.yaml @@ -0,0 +1,168 @@ +id: buildoor-testing-build +name: "buildoor testing build: planned transactions land on chain exactly" +description: | + Switches a buildoor to its testing build source (blocks built from the + builder's private tx intake through geth's testing_buildBlockV1), feeds the + intake with a spamoor scenario, and requires that every buildoor block after + the switch verified as an exact tx plan match: same transactions, same + order, same count. A single mismatch, missed payload or unknown block fails + the test. + + Endpoint setup. While the builder is on the testing source its blocks are + the ONLY way a transaction gets included: anything sent to a normal EL sits + in the public txpool forever, because buildoor builds solely from its + intake. That includes spamoor's own wallet funding and refills, which use + the group-less "default" selection rather than this scenario's client_group. + + So every endpoint spamoor may pick must be the intake. The simple form is to + give this playbook a config where the intake is the only execution endpoint + (it proxies all reads to the EL, so the client checks still work). It still + needs the group this test's clientGroup names, since a scenario selecting a + group no client is in gets no client at all: + + endpoints: + - name: buildoor-intake + executionUrl: http://buildoor:8080/rpc + consensusUrl: http://beacon:5052 + executionGroups: [builder] + + With several execution endpoints, keep the intake in "default" and take the + others out of it, so the scenario reaches every node it needs while funding + can only land somewhere that gets built (needs a spamoor with group removal, + ethpandaops/spamoor#283): + + endpoints: + - name: buildoor-intake + executionUrl: http://buildoor:8080/rpc + consensusUrl: http://beacon:5052 + executionGroups: [builder] + - name: node-2 + executionUrl: http://node2:8545 + consensusUrl: http://beacon2:5052 + executionGroups: ["-default"] + + The buildoor geth must serve the testing namespace (--http.api ...,testing). +version: 1.0.0 +tags: [buildoor, epbs, load, testing-build] +timeout: 30m +config: + buildoorUrl: "http://buildoor:8080" + spamoorPrivkey: "" + clientGroup: "builder" + minVerifiedBlocks: 6 + minTxsPerBlock: 20 + throughput: 300 + baseFeeGwei: 100 + # spamoor's scenarioYaml takes these in WEI (the --refill-* CLI flags take + # ETH, the YAML fields do not). Too small and the wallets are "funded" with + # dust, pass the balance check, and every transaction is then skipped for + # insufficient funds. + # Quoted: jq evaluates configVars as float64, so an unquoted 5e19 reaches + # spamoor as "5e+19" and fails its uint256 parse. + refillAmountWei: "50000000000000000000" # 50 ETH + refillBalanceWei: "10000000000000000000" # 10 ETH +tasks: +- name: check_clients_are_healthy + title: "Check if at least one client is ready" + timeout: 5m + config: + minClientCount: 1 + +- name: check_http_json + title: "Switch buildoor to the testing build source" + timeout: 1m + configVars: + url: ".buildoorUrl + \"/api/config/testing\"" + config: + method: POST + body: + source: testing + fill_gas_pct: 100 + policy: fifo + expectStatus: 200 + failOnCheckMiss: true + assertions: + - name: updated + query: ".status" + operator: eq + value: "updated" + +- name: run_task_background + title: "Feed the intake and verify buildoor's blocks" + config: + onBackgroundComplete: fail + backgroundTask: + name: run_spamoor_scenario + title: "spamoor eoatx into the builder intake" + configVars: + privateKey: "spamoorPrivkey" + scenarioYaml: ". as $v | {throughput: $v.throughput, max_pending: ($v.throughput * 10), max_wallets: 30, base_fee: $v.baseFeeGwei, tip_fee: 2, refill_amount: $v.refillAmountWei, refill_balance: $v.refillBalanceWei, rebroadcast: 30, client_group: $v.clientGroup}" + config: + scenarioName: eoatx + foregroundTask: + name: run_tasks + title: "Verify" + config: + tasks: + - name: check_consensus_block_proposals + title: "buildoor proposes ${minVerifiedBlocks} blocks with >= ${minTxsPerBlock} transactions" + timeout: 20m + configVars: + blockCount: "minVerifiedBlocks" + minTransactionCount: "minTxsPerBlock" + config: + checkLookback: 64 + extraDataPattern: "^buildoor" + + # Two checks on purpose. The tally lags the proposals it counts (the + # verifier fetches the block from the EL after inclusion), and an + # unsatisfied assertion is a FAILURE rather than a wait under + # failOnCheckMiss - so the count is polled without it, and only the + # must-be-zero counters fail fast. + - name: check_http_json + title: "Wait for ${minVerifiedBlocks} verified tx plans" + timeout: 5m + configVars: + url: ".buildoorUrl + \"/api/buildoor/tx-queue\"" + assertions: >- + . as $v | [ + {name: "enough-matches", query: ".plan_checks.match", operator: "gte", value: $v.minVerifiedBlocks} + ] + config: + pollInterval: 5s + + - name: check_http_json + title: "No tx plan was mismatched, missed or unverifiable" + timeout: 1m + configVars: + url: ".buildoorUrl + \"/api/buildoor/tx-queue\"" + # Cumulative counters, so this covers the whole run, not just now. + # Leading "." matters: assertoor prepends one to a configVars + # expression that lacks it, turning a bare [..] into an index. + assertions: >- + . | [ + {name: "no-mismatch", query: ".plan_checks.mismatch", operator: "eq", value: 0}, + {name: "no-missing-blocks", query: ".plan_checks.block_not_found", operator: "eq", value: 0}, + {name: "no-missed-payloads", query: ".plan_checks.missed", operator: "eq", value: 0} + ] + config: + failOnCheckMiss: true + + - name: check_consensus_forks + title: "No forks" + timeout: 1m + config: + minCheckEpochCount: 1 + maxForkDistance: 1 + +cleanupTasks: +- name: check_http_json + title: "Switch buildoor back to the pool build source" + timeout: 1m + configVars: + url: ".buildoorUrl + \"/api/config/testing\"" + config: + method: POST + body: + source: pool + expectStatus: 200