Ccq/p2p with single host

[bruce-riley]

This PR attempts to switch CCQ to use the same host / bootstrap parameters as regular gossip, but different pub/sub channels.

To test this:

• Bring up tilt with tilt up -- --manual (Filter for ccq in the guardian and you should see the ccq server initialializing.)
• Run the test by cd'ing to node/hack/query and doing kubectl --namespace=wormhole exec -it spy-0 -- sh -c "cd node/hack/query/ && go run send_req.go".

You should see the request being received in the guardian and the response being published by the guardian. However, the test will hang waiting on the response (last message in stdout is "Waiting for message").

[SEJeff]

Some comments before you split this all out. I'd love to see more tests for the business logic, but this is great.

External facing services should ideally have:

• Unit test coverage of the positive and negative code paths
• Metrics for rpcs, errors, etc
• Logging of requests (middleware for the webserver).

[SEJeff]

Suggested change

	if (r.senderChainId == 0) {
	// 65 byte sig for off-chain, 32 byte vaaHash for on-chain
	if (r.senderChainId == 0) {

Maybe it is worth reiterating the comment here for clarity?

[SEJeff]

Up to 255 iterations seems worth saving a bit of gas by both preincrementing the idx variable and doing it in an unchecked block. It is bounded, so there is no risk of overflow.

Suggested change

	for (uint idx = 0; idx < numPerChainQueries; idx++) {
	r.responses[idx].chainId = response.toUint16(reqIdx);
	require(response.toUint16(respIdx) == r.responses[idx].chainId, "reqChainId does not match respChainId");
	reqIdx += 2;
	respIdx += 2;
	r.responses[idx].queryType = response.toUint8(reqIdx);
	require(response.toUint8(respIdx) == r.responses[idx].queryType, "reqQueryType does not match respQueryType");
	reqIdx += 1;
	respIdx += 1;
	require(r.responses[idx].queryType == 1, "EthCall is the only supported query type");
	len = response.toUint32(reqIdx);
	reqIdx += 4;
	r.responses[idx].request = response.slice(reqIdx, len);
	reqIdx += len;
	len = response.toUint32(respIdx);
	respIdx += 4;
	r.responses[idx].response = response.slice(respIdx, len);
	respIdx += len;
	}
	for (uint idx = 0; idx < numPerChainQueries;) {
	r.responses[idx].chainId = response.toUint16(reqIdx);
	require(response.toUint16(respIdx) == r.responses[idx].chainId, "reqChainId does not match respChainId");
	reqIdx += 2;
	respIdx += 2;
	r.responses[idx].queryType = response.toUint8(reqIdx);
	require(response.toUint8(respIdx) == r.responses[idx].queryType, "reqQueryType does not match respQueryType");
	reqIdx += 1;
	respIdx += 1;
	require(r.responses[idx].queryType == 1, "EthCall is the only supported query type");
	len = response.toUint32(reqIdx);
	reqIdx += 4;
	r.responses[idx].request = response.slice(reqIdx, len);
	reqIdx += len;
	len = response.toUint32(respIdx);
	respIdx += 4;
	r.responses[idx].response = response.slice(respIdx, len);
	respIdx += len;
	unchecked { ++idx; }
	}

Proof it is more gas efficient:

$ cat test/Gas.t.sol
// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import {Test} from "forge-std/Test.sol";

contract GasSaver is Test {
    uint256 constant private max = 255;

    function testIndexIncrement() public pure {
        for (uint i = 0; i < max; i++) {
        }
    }
    function testIndexPreIncrementUnchecked() public pure {
        for (uint i = 0; i < max;) {
            unchecked { i += 1; }
        }
    }
}

$ forge test --match-contract GasSaver
[⠘] Compiling...
No files changed, compilation skipped

Running 2 tests for test/Gas.t.sol:GasSaver
[PASS] testIndexIncrement()             (gas: 28482)
[PASS] testIndexPreIncrementUnchecked() (gas: 11164)
Test result: ok. 2 passed; 0 failed; 0 skipped; finished in 1.02ms
Ran 1 test suites: 2 tests passed, 0 failed, 0 skipped (2 total tests)

[SEJeff]

Same as before. Consider preincrementing idx vs postincrementing and putting it in an unchecked block to save the caller of this method some gas.

[SEJeff]

You're not modifying the signatures variable and it is an array, so you might consider changing to calldata instead of memory to save gas. This is of course if this API isn't already set in stone.

[SEJeff]

In some of the code you use require($condition, "revert text here"); and in this code you use a conditional and then a revert with a string.

Solidity will return the 4 byte function selector hash for custom error messages making it generally more gas efficient. In functions like parseAndVerifyQueryResponse() where you have require error strings that are greater than 32 bytes, it can be quite a bit more gas efficient to use custom errors.

Whichever of the three approaches you use, consider trying to be consistent. The three options:

• revert CustomError()
• revert("some string")
• require(statement, "some descriptive string");

[SEJeff]

Consider defering a client close, like how they do it in the go-ethereum unit tests for rpc.

Suggested change

	}
	}
	defer rawClient.Close()

[SEJeff]

Suggested change

	}
	}
	defer rawClient.Close()

[SEJeff]

Suggested change

	logger.Fatal("failed to create chain governor", zap.Error(err), zap.String("component", "ccq"))
	logger.Fatal("failed to create query handler", zap.Error(err), zap.String("component", "ccq"))

[SEJeff]

Seems like this could be shared between ccq and the main guardian codebase that does this?

[SEJeff]

Can this be a more descriptive error? Preferably one that doesn't require reading the source and finding the definition of h in the function arg spec?

[SEJeff]

These comments are ones I made several days ago, but forgot to submit. I'm going to submit them now and then try to reproduce your issue.