integration: prove a 2-of-2 endorsement policy over a real backend - #306
Open
Storm1289 wants to merge 15 commits into
Open
integration: prove a 2-of-2 endorsement policy over a real backend#306Storm1289 wants to merge 15 commits into
Storm1289 wants to merge 15 commits into
Conversation
- add a separate basic2of2 namespace/policy to start-x, alongside the existing single-org one - add fabx-2of2.yaml wiring org1 and org2 as endorsers - add a TestFabricX subtest that commits through the real committer under the AND(Org1MSP.member, Org2MSP.member) policy Signed-off-by: Storm1289 <divakarsharm2934@gmail.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #306 +/- ##
==========================================
- Coverage 29.56% 29.26% -0.30%
==========================================
Files 79 80 +1
Lines 11177 11303 +126
==========================================
+ Hits 3304 3308 +4
- Misses 7579 7700 +121
- Partials 294 295 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Storm1289
marked this pull request as ready for review
August 7, 2026 13:02
Contributor
Author
|
hi @arner, whenever you have sometime please review this pr :) |
- oz-hardhat-compat and quick-tests both get more headroom (the full OZ suite alone was hitting the old 25m ceiling) - record-PR-number step now creates its own artifact dir, so it can't fail when an earlier step is skipped or the job is cancelled Signed-off-by: Storm1289 <divakarsharm2934@gmail.com>
✅ OZ Hardhat Compatibility — 84.4% passing (5143/6094)No regressions, no stale entries. Full breakdown — 5143 passed, 950 failed, 1 skipped (6094 total, 84.4% passing)By suite
Expected failures by cause (950)
|
added 3 commits
August 7, 2026 17:08
Governor using $ERC20VotesTimestampMock's 'signature does not match signer' case was never recorded, unlike its five sibling variants with the same underlying cause. Flagged as a CI regression on an unrelated PR; not caused by that PR's changes. Signed-off-by: Storm1289 <divakarsharm2934@gmail.com>
Governor $ERC20VotesTimestampMock's signature-mismatch case failed locally and in the CI run that flagged it, but passes on this run - appears flaky/non-deterministic, not related to this PR's changes. Signed-off-by: Storm1289 <divakarsharm2934@gmail.com>
Governor $ERC20VotesTimestampMock's signature-mismatch case has failed on two runs (CI, local) and passed on one - flaky rather than deterministic. Keeping it in the baseline for now; leaving the call on the underlying flakiness to maintainer review. Signed-off-by: Storm1289 <divakarsharm2934@gmail.com>
Contributor
Author
|
Hi @arner, heads up for the OZ Hardhat baseline gate looks flaky around *TimestampMock Governor tests (ERC20 and ERC721 variants) - same tests flipping pass/fail across runs, likely tied to the documented fixed-timestamp limitation. Not related to this PR (Makefile/CI/integration-test only). Let me know how you'd like to handle it. |
Contributor
|
Thanks @Storm1289 you're right about the flaky tests (though not sure if the timestamp is the issue - that should be consistent). I think it may have to do with how hardhat tests are forced to be sequential (and build on the previous tx result), even when pushed at almost the same time. That would be exactly the kind of issue that the dependency manager should be able to solve! In the meantime I'm considering to add a flakiness flag to the tests; right now we can't really deal with flapping tests, we expect either failure or success... |
Contributor
|
It looks good, but these are two embedded endorsers. I was hoping that we can make a test over GRPC, with two instances of an endorser (can be the full application including gateway). It would at least mean two config files; one for each organization. That would make it a step closer to a real, multi-org deployment. We can still run it in one process (easy to start with the integration tests). At some later point we should also add a sample deployment with several organizations, and if we consider it worthwhile we could test against that too. What do you think? |
- Run org1 and org2 as standalone endorsers behind their own mTLS gRPC servers, with a split-deployment gateway dialing both. - Carry the signed endorsed result in ExecuteResponse.read_write_set; without it the packaged tx was empty and rejected after ordering. - Assert endorsed-payload parity across the gRPC boundary. Signed-off-by: Storm1289 <divakarsharm2934@gmail.com>
Contributor
Author
|
Thanks @arner for the review. This surfaced a bug in the merged gRPC path: the endorsed payload wasn't crossing the wire, so split deployment could endorse and submit but never commit. Two lines to fix, plus the parity-test assertion that would have caught it. Kept it here since the new test can't pass without it. Could you add the |
arner
reviewed
Aug 10, 2026
- Rename the top-level `endorsers` list to a single `endorser`, so it no longer collides with `gateway.endorsers` for remote ones, and reject configs that set both. - Move the Fablo suite to two standalone endorsers over gRPC, since its chaincode needs both orgs to endorse. - Apply tls.server-name when dialing an endorser; without it verification used the dial address and rejected hostname-issued certs. Signed-off-by: Storm1289 <divakarsharm2934@gmail.com>
- Serve TLS with a certificate carrying no IP SANs, as real peer certs do, and assert tls.server-name is what gets verified. - Without it the dial address is verified instead, which is the case that silently broke reaching an endorser by IP. Signed-off-by: Storm1289 <divakarsharm2934@gmail.com>
Contributor
Author
|
Hi @arner, updated the PR description. All three points are in: |
arner
approved these changes
Aug 11, 2026
arner
left a comment
arner
left a comment
Contributor
There was a problem hiding this comment.
LGTM! @ale-linux can you take a look as well? Maybe we can discuss the setup in our call. The "mixed mode" should not be too hard to add and we can consider going for that for at least org1 (so we have only two config files per org). But that would be a separate PR anyway IMO.
Reword per review; "yet" reflects that mixed mode is planned separately. Signed-off-by: Storm1289 <divakarsharm2934@gmail.com>
ale-linux
requested changes
Aug 13, 2026
ale-linux
left a comment
ale-linux
left a comment
Contributor
There was a problem hiding this comment.
Great stuff! Just a couple of small improvements
| name: Quick Tests | ||
| runs-on: ubuntu-latest | ||
| timeout-minutes: 15 | ||
| timeout-minutes: 35 |
Contributor
There was a problem hiding this comment.
Hmm - a quick test requiring 35 min... 🤔 can we not have this?
| done; \ | ||
| [ "$$ok" = 1 ] || { echo "Error: namespace setup failed after 5 attempts"; exit 1; } | ||
| @echo "Creating 2-of-2 namespace (retrying until the committer is ready)..." | ||
| @ok=0; for attempt in 1 2 3 4 5; do \ |
Contributor
There was a problem hiding this comment.
This appears a bit brittle - can we do something a bit more resilient?
Signed-off-by: Storm <divakarsharma2934@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #302. Related to #21 (Split Gateway and Endorser Services).
End-to-end proof that a 2-of-2 endorsement policy is accepted by the real Fabric-X backend, with each organization's endorser reached over real gRPC. Includes the
endorser/gateway.endorsersnaming change from review, the Fablo setup moved to the same shape, and two bugs this surfaced in the already-merged split-deployment path.Files
Added
integration/fabx-2of2.yamlintegration/fabx-2of2-org1.yamlendorser)integration/fabx-2of2-org2.yamlendorser)integration/fablo-org1.yamlintegration/fablo-org2.yamlintegration/split_endorsers.goModified
gateway/config/config.goEndorsers []becomesEndorser *; reject both keys set, or neithergateway/app/app.gonewAppbuilds one embedded endorser instead of a listendorser/client/dial.gotls.server-namewhen dialing (fix 2)endorser/client/client.goendorser/server/handlers.gointegration/fablo.yamlintegration/fabx.yaml,fabx-full.yaml,fablo.hardhat.yamlendorsers:renamed toendorser:integration/test_helpers.gonewSplitFileConfigHarness; build the single embedded endorserintegration/integration_test.goTestFablouses the split harness; 2-of-2 subtestintegration/endorsement_grpc_test.goendorser/client/dial_test.gogateway/config/validate_test.go,load_test.goMakefilestart-xalso creates thebasic2of2namespace.github/workflows/tests.ymlConfig:
endorsersingular,gateway.endorserspluralendorserspreviously meant two different things. Nowendorser(singular, top level) is this process's own embedded endorser, andgateway.endorsers(plural) are remote ones dialed over gRPC. Setting both is rejected. Mixed mode is deliberately not implemented, per review.Since the field is a single object rather than a list, no config can embed more than one endorser. A file per organization is not the same as embedding several: each org file declares exactly one
endorser, and they run as separate endorsers that a gateway dials.Test setup
Fabric-X - org1 and org2 each run as a standalone endorser behind its own mTLS gRPC server, using that org's real
endorser.<org>.example.commaterial, with a split-deployment gateway dialing both. Drives the realgateway/app.App, the same typefxevm startuses.start-xcreates abasic2of2namespace withAND('Org1MSP.member','Org2MSP.member'), separate frombasicso the stricter policy doesn't affect the existing single-org cases.Fablo - split the same way. Its chaincode declares no endorsement policy, so Fablo commits it with
AND('Org1MSP.member','Org2MSP.member')and both orgs must endorse, which is why it embedded two endorsers before and could not stay singular.fabx.yamldeliberately stays embedded with a singleendorser, so the embedded deployment mode keeps end-to-end coverage against a real backend.newSplitFileConfigHarnesskeeps each endorser's KVS registered as a block handler and its builder available to the state primer, sotether_token_replaystill works, while only the gateway's path to them is gRPC - the part under test.Fix 1: the signed endorsement payload was dropped over gRPC
peer.ProposalResponsehas a top-levelPayload- the signed resultfabricx.CreateTxassembles the transaction from - which is a different field fromResponse.Payload(EVM return data). Only the latter crossed the wire, so the top-level one arrived empty: 456 bytes in-process, 0 bytes over gRPC.An empty payload still unmarshals cleanly, so nothing reported an error: the gateway packaged an empty transaction, the orderer accepted it and cut a block, and it was then rejected. Split deployment could endorse and submit but never commit.
Fixed by carrying it in
ExecuteResponse.read_write_set, the fielddocs/design/endorsement-api/01-api-and-proto.mdalready specifies for this ("kept byte-exact for signing"), so no proto change was needed.TestGRPCEndorsement_Parityhad compared onlyResponse.Payload- empty on both sides, so trivially equal - and now asserts the signed payload is non-empty and identical across the boundary.Fix 2:
tls.server-namewas ignored when dialing an endorserClientConfig.TLS.ServerNameexists and bothToPeerConfandToOrdererConfpropagate it, buteclient.Dialdropped it, so certificates were verified against the dial address:x509: cannot validate certificate for 127.0.0.1 because it doesn't contain any IP SANsThis stayed hidden because the Fabric-X test endorser certs happen to carry
IP:127.0.0.1SANs. Real peer certificates do not - Fablo's areDNS:peer0.orgN.example.comonly - so any deployment whose endorser cert is issued for a hostname and reached by IP would fail. Fixed withgrpc.WithAuthority, covered by a test that serves a hostname-only certificate and fails with that exact error without the fix.Verification
Against real backends, not mocks:
TestFabricX11/11 (including the newtwo_of_two_endorsement_policy) andTestFablo10/10, both on freshly created networks, re-run after rebasing onto themainmerges that brought in #305 and #294.make checksclean and the full-shortsuite green.tether_token_replayrequires a fresh network in both suites - it primes state and replays a historic mainnet transaction. That behaviour is unchanged here and reproduces identically onmain.Test plan
go build ./.../go vet ./.../gofmt -l(clean)make checksgo test -short ./...make test-x(11/11)make test-fablo(10/10)