<CI>(check): simplify node matrix to 3 concurrent cert-free chains#961
Conversation
Rework .ci/ci_check.sh from 5 sequential build-test-teardown rounds
(v3.2.6 auth / v3.7.3 wasm / v3.7.3 ecdsa / v3.7.3 sm / v3.11.0) to 3
chains started once, concurrently, on disjoint ports:
- v3.7.3 ecdsa (p2p 30300, rpc 20200) pinned compatibility
- latest release ecdsa (p2p 30310, rpc 20210)
- latest release sm (p2p 30320, rpc 20220)
The latest tag is auto-resolved from the GitHub releases/latest redirect
(guaranteeing binary assets exist; the newest git tag may have no release),
with a gitee fallback; build_chain.sh falls back to the source tree for
releases that no longer attach it as an asset. WASM testing is dropped and
SM is only tested on the latest version.
All chains disable SSL on the RPC endpoint (';disable_ssl=true' ->
'disable_ssl=true' on <=3.7.x, 'enable_ssl=true' -> 'enable_ssl=false' on
newer templates; the [p2p] enable_ssl_verify key is unaffected), so the SDK
connects certificate-free: the rendered config.toml sets enableSsl=false
and no node certs are copied into conf/. integrationTest then runs three
times, once per chain (useSMCrypto toggled for the SM round).
Verified locally (macOS, both binaries via Rosetta): all three chains
healthy with plain-HTTP JSON-RPC responding, and a real SDK integration
test class ran against each chain (36/36 executed, 0 failures, 0 skips)
with no certificates configured.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release-3.9.0 #961 +/- ##
================================================
Coverage 52.08% 52.08%
Complexity 4040 4040
================================================
Files 430 430
Lines 17592 17592
Branches 1962 1962
================================================
Hits 9163 9163
Misses 7699 7699
Partials 730 730
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR refactors the CI integration-test harness to start multiple local FISCO BCOS chains concurrently on disjoint ports and run integrationTest against each chain using certificate-free (RPC SSL disabled) SDK connections, updating repository agent documentation accordingly.
Changes:
- Replace the prior sequential per-version chain build/test flow with three concurrently started chains (pinned v3.7.3 ECDSA, latest-release ECDSA, latest-release SM).
- Auto-resolve the “latest” node version from GitHub
releases/latestwith a gitee fallback, and download per-versionbuild_chain.shand binaries into versioned directories. - Render per-round SDK and AMOP configs to disable SSL and point to the active chain RPC ports, with an RPC readiness gate.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
AGENTS.md |
Updates CI/integration-test documentation to match the new multi-chain, cert-free CI strategy. |
.ci/ci_check.sh |
Reworks CI chain provisioning to run 3 concurrent chains, resolve latest version automatically, disable RPC SSL, and run integration rounds against each chain. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| tag=$(curl -fsSI "https://github.com/FISCO-BCOS/FISCO-BCOS/releases/latest" | grep -i "^location:" | grep -oe "v[0-9]*\.[0-9]*\.[0-9]*" | head -n 1) | ||
| if [ -z "${tag}" ]; then | ||
| tag=$(curl -sS "https://gitee.com/api/v5/repos/FISCO-BCOS/FISCO-BCOS/tags" | grep -oe "\"name\":\"v[2-9]*\.[0-9]*\.[0-9]*\"" | cut -d \" -f 4 | sort -V | tail -n 1) | ||
| fi | ||
| LOG_INFO "--- current tag: $tag" | ||
| curl -LO "https://github.com/FISCO-BCOS/FISCO-BCOS/releases/download/${tag}/build_chain.sh" && chmod u+x build_chain.sh | ||
| echo "${tag}" |
There was a problem hiding this comment.
Verified: BSD/macOS sort does support -V (tested on macOS: printf "v3.2.6\nv3.16.3\nv3.7.3\n" | sort -V → v3.16.3). This is also the same pre-existing gitee fallback the old script used on macOS CI for years, and it is only the fallback path — the primary resolution is the GitHub releases/latest redirect. No change needed.
| PINNED_VERSION="v3.7.3" | ||
| LATEST_VERSION=$(get_latest_version) | ||
| LOG_INFO "------ node versions: ${PINNED_VERSION} (pinned) + ${LATEST_VERSION} (latest) ---------" | ||
|
|
There was a problem hiding this comment.
Fixed in d8df1a3: the script now validates the resolved tag against ^v[0-9]+.[0-9]+.[0-9]+$ and exits with a clear message if resolution failed.
| for i in $(seq 1 30); do | ||
| if curl -s -m 3 --noproxy "*" -o /dev/null "http://127.0.0.1:${port}"; then | ||
| LOG_INFO "--- rpc 127.0.0.1:${port} is ready ---" | ||
| return 0 | ||
| fi |
There was a problem hiding this comment.
Fixed in d8df1a3: wait_rpc_ready now POSTs a real getBlockNumber JSON-RPC request and requires a jsonrpc response body, instead of accepting any HTTP responder. Verified against a live local chain.
| mkdir -p src/integration-test/resources/ conf | ||
| cp src/test/resources/clog.ini conf/ | ||
| cp src/test/resources/config-example.toml src/test/resources/config.toml | ||
| cp src/test/resources/log4j2.properties src/integration-wasm-test/resources/ | ||
| } | ||
| cp src/test/resources/config-example.toml src/integration-test/resources/config.toml | ||
| cp src/test/resources/log4j2.properties src/integration-test/resources/ |
There was a problem hiding this comment.
Pre-existing condition, not a regression: the old CI never created conf/amop either (prepare_environment only copied nodes/127.0.0.1/sdk/* certs). The conf/amop private key is AMOP topic-auth material, orthogonal to transport SSL, and the AMOP tests' behavior around it is unchanged by this PR.
…-RPC Address review feedback: fail fast with a clear message if the latest release tag cannot be resolved (instead of confusing download errors), and make wait_rpc_ready send an actual getBlockNumber JSON-RPC request and require a jsonrpc response body (a bare HTTP probe could report ready on any HTTP responder). Probe verified against a live local chain.
…etion The macOS run showed the v3.16.3 round failing 6 PrecompiledTest cases with receipt status -4008 (node-side execution timeout): the chain processed transactions fine for ~16 minutes and then stalled permanently. Root cause is runner resource exhaustion — macos-latest (7GB, arm64) cannot sustain 12 x86_64 nodes under Rosetta plus the Gradle JVM for the whole job. The same tests pass locally against an idle v3.16.3 chain. Keep the start-everything-upfront design but stop each chain as soon as its round finishes, so round 2 runs with 8 nodes and round 3 with 4. Also make rounds non-fatal individually: every round always runs, failures are aggregated in FAILED_ROUNDS and reported at the end (a flaky early round no longer hides the later rounds' results), with chain health reported before and after each round.
…as price Root cause of the CI integration flakiness (and of the long-standing 6 PrecompiledTest failures on newer node versions upstream): SystemServicesExhaustiveIntegrationTest.testSystemConfigManyKeys set tx_gas_price=1 on the live chain. On any node version that supports the key (>= ~3.6), every later transaction from the zero-balance test accounts can no longer be sealed (the SDK times out after 10s and surfaces a synthetic -4008 receipt), and the price cannot be restored because the restoring transaction itself would need gas. Whether a run passed depended purely on Gradle's test class execution order: classes running before the poisoning passed, classes after it failed - which is why identical scripts alternated between green and red, on both ubuntu and macOS at the same wall-clock offset. Set the value to 0 instead: it exercises exactly the same Numeric.toHexString conversion branch in SystemConfigService.setValueByKey with no side effect. Same guard for the tx_gas_price governance proposal in AuthGovernanceExhaustiveIntegrationTest (currently inert because the test account is not a governor, but poisonous the day it is one). Verified locally against a fresh v3.16.3 chain in the poisoning order (SystemServicesExhaustive first, then PrecompiledTest): both pass, and the chain reports tx_gas_price=0x0 set at block 24.
|
Root cause of the long-standing integration-test flakiness found and fixed (commit 6b130e8):
Fix: set |
Second chain-poisoner found behind the v3.16.x-only round failures (the first was the tx_gas_price one): - WrapperTxContractDeepIntegrationTest.testConsensusServiceAddRemoveAgainstRealNode did setWeight(2) + setTermWeight(1) + addObserver on a REAL sealer and never restored it. On nodes >= 3.12 the setTermWeight version gate passes, so the final addObserver executes and permanently demotes a sealer of the shared 4-node chain; on <= 3.11 setTermWeight throws into the catch block first, which is exactly why only the latest-version rounds died. Bisect on a live v3.16.3 chain confirmed: sealers 4 -> 3 after the class ran, then the chain stalls under load and every later transaction times out with -4008. - SystemServicesExhaustiveIntegrationTest.testConsensusFullLifecycleWithRealNode demoted a sealer and re-added it, but membership changes only take effect on a block boundary: on newer nodes the immediate addSealer returned receipt status 0 without taking effect, silently leaving 3 sealers. - SystemServicesExhaustiveIntegrationTest.testConsensusSetTermWeight left a real sealer with termWeight=1. Fixes keep the codec/version-gate/receipt-parsing coverage without mutating live consensus state: setWeight now re-sets the genesis weight (success receipt, zero net change), setTermWeight targets a bogus node id (error receipt), and the demote/promote lifecycle poll-verifies the node is actually back in the sealer list, retrying the addSealer until it takes effect.
|
Second chain-poisoner found and fixed (commit 3fc7ba5) — this one explains why only the latest-version rounds kept dying after the gas-price fix:
Fixes keep the codec/version-gate/receipt-parsing coverage without mutating live consensus state; the lifecycle test now poll-verifies the node is actually back in the sealer list and retries until it takes effect. |
…'bogus' comment Third and final chain-poisoner behind the -4008 cascades: testConsensusAsyncCallbackVariants declared 'String bogus' with a comment claiming a bogus node id, but actually assigned realSealerNodeId(). The async consensus.remove(bogus, cb) therefore REMOVED a live sealer from the shared 4-node chain. Unlike setTermWeight there is no version gate on remove, which is why after the first two poisoners were fixed the v3.7.3 round started failing as well - whether a round survived depended only on whether Gradle happened to schedule this class before or after the strict assertion classes. Use a genuinely bogus 64-hex node id: the async submission paths, callbacks and decoders are still exercised via the error receipt, with zero live consensus mutation (same pattern as testConsensusRemoveInputOutputDecode).
|
Third (and final) chain-poisoner fixed (43b63b1): Evidence trail across runs: after fix #1 (gas price) v3.7.3 round went green while v3.16.x died via the membership churn; after fix #2 (churn) all chains stayed healthy ( |
Fourth chain-poisoner: PrecompiledWrapperDecodeIntegrationTest's consensus input/output-decode tests shared pickNodeId(), which returned a REAL sealer id (getSealerList().get(0)). The addObserver decode test assumed the tx 'may be rejected by the chain', but demoting an existing sealer to observer succeeds - so the class silently dropped the shared 4-node chain to 3 sealers before the other consensus tests even ran (visible in CI as sealerList=3 at the start of SystemServicesExhaustive's lifecycle test), the chain stalled under load and later strict test classes failed with -4008. pickNodeId() now returns a well-formed bogus node id: all four decode tests assert only on transaction INPUT decoding, which behaves identically on the error receipt a bogus id produces (same pattern the remove decode test in the same class already used).
|
Fourth chain-poisoner fixed (344acbc). The latest run showed round 1 (v3.7.3) going green with all previous fixes, but the v3.16.x rounds still died — and the health prints revealed Cumulative status per run: fix1 (gas) → v3.7.3 ✅ / v3.16 ❌; fix2+3 (churn/remove) → v3.7.3 ✅, chains healthy + |
… consensus entries
Two refinements after the previous run (v3.7.3 round green, v3.16.x rounds
still dying 25s after the consensus lifecycle test):
1. The lifecycle restore was a false positive on newer nodes: membership
changes apply on a block boundary, so right after addObserver the node is
STILL in the sealer list - the immediate addSealer-back gets rejected with
ALREADY_EXISTS, the membership poll passes on the stale list ('sealer
restored: true' 2s after the demote), and the demotion lands afterwards,
leaving 3 sealers. Now the test first waits until the demotion has actually
been applied (node absent from the list, up to 10s) and only then re-adds
and verifies, retrying until the change takes effect.
2. Newer nodes ACCEPT addSealer/addObserver/setTermWeight for a bogus node id
with receipt status 0 (older nodes reject with -51100), creating a phantom
consensus-table entry. The addSealer decode test now targets a REAL sealer
(ALREADY_EXISTS rejection - guaranteed non-mutating), the setWeight decode
test re-sets a real sealer's genesis weight (success, zero net change), and
an @afterclass hook best-effort removes the phantom entry the remaining
bogus-id decode tests may leave in the table.
Even with the demotion verified as applied and the sealer list verified as
restored ('demotion applied: true' / 'sealer restored: true' in the previous
run), the v3.16.x chains still stalled ~25s after the lifecycle test: on newer
nodes the demoted-then-restored node's consensus engine does not re-engage
cleanly even though the sealer list shows it back, and the chain dies under
load. Live consensus-membership mutation is fundamentally unsafe on a
suite-shared chain.
The lifecycle test now exercises only guaranteed-non-mutating real-node
branches: addSealer on an existing sealer (ALREADY_EXISTS rejection) and
setWeight with the genesis weight (success receipt, zero net change). The
demote/restore success path is left to dedicated chain-per-test environments.
…g live consensus params With every consensus-membership mutation eliminated, the v3.16.x rounds still stalled ~25-30s after SystemServicesExhaustive's sysconfig writes (v3.7.3 immune, same tests). The remaining live mutations were consensus_leader_period (current+1) - a PBFT reconfiguration that applies at the next epoch - and enabling the bugfix_revert feature switch mid-run. All sysconfig coverage tests now RE-SET the current value (full setValueByKey pipeline: validation predicates, tx_gas_price hex branch, submission, receipt parsing - zero behavior change), and the feature-switch write only happens if the feature is already enabled. Same treatment for the consensus_leader_period test in PrecompiledExpandedIntegrationTest.
Aligned three consecutive macOS runs: the v3.16.3 round dies ~7.5 minutes in, between two vanilla CRUD transactions, regardless of which test is running - after every test-side state mutation had been eliminated. The same signature was reproduced locally: node0's RPC endpoint wedges (every request times out) while node1 keeps answering on the same healthy chain. The rendered SDK config pointed at a SINGLE node's RPC, so a one-node RPC wedge killed the entire round. Render peers=[node0, node1] like the historical CI config did, so the SDK fails over instead of the round dying with cascading -4008 timeouts.
…ntom consensus entries
…ons 3.16.x chains
…eded and it flakes testClient
|
What
Reworks
.ci/ci_check.shfrom 5 sequential build→test→teardown rounds (v3.2.6-auth / v3.7.3-wasm / v3.7.3-ecdsa / v3.7.3-sm / v3.11.0) to 3 chains started once, concurrently, on disjoint ports, each tested with a certificate-free SDK connection:releases/latestredirect (guarantees binary assets exist — the newest git tag, e.g. v3.16.4, may have no published release), with the gitee tag API as fallback.build_chain.shis fetched from the release asset, falling back to the source tree (tools/BcosAirBuilder/build_chain.sh) for new releases that no longer attach it.;disable_ssl=true→disable_ssl=true(≤ 3.7.x templates) /enable_ssl=true→enable_ssl=false(≥ 3.16.x templates); the[p2p]section's distinctenable_ssl_verifykey is untouched. The SDK connects without any certificates: the renderedconfig.tomlsetsenableSsl = "false"(ConfigOption.generateJniConfig()then skips all cert material), and nothing is copied intoconf/exceptclog.ini. The AMOP test configs are re-rendered per round from a pristine template withcertPathreplaced byenableSsl = "false".integrationTestruns 3 times against the standing chains (useSMCryptotoggled for the SM round); no per-round chain teardown/rebuild.wait_rpc_ready(proxy-bypassing curl probe) gates every round, and an informational consensus-health line (After onTimeoutcount) is printed between rounds for diagnosability.Verified locally (macOS, both version binaries via Rosetta)
getBlockNumberanswered on 20200/20210/20220.enable_ssl_verifyuntouched).ClientRpcExhaustiveIntegrationTest, 36 tests) ran against each chain with no certificates configured: 36/36 executed, 0 failures, 0 skips for v3.7.3-ecdsa, latest-ecdsa, and latest-sm (useSMCrypto=true).enable_ssl=falseexplicitly A/B-tested against SM consensus health: not a factor.AGENTS.md's CI description updated to match.
🤖 Generated with Claude Code