evm: per chain transceivers #517

bruce-riley · 2024-09-12T19:00:36Z

This PR implements the NttManagerWithPerChainTransceivers contract which inherits from NttManagerNoRateLimiting and allows configuring different transceivers and thresholds for each chain. You can configure a different set of send and receive transceivers for each chain. If you don't specifically enable any transceivers for a chain, then all transceivers will be used.

This new contract has to inherit from the no-rate-limiting due to the contract size limit.

bruce-riley · 2024-09-25T15:27:24Z

Contract sizes before this feature:

| NttManager                            |   23,615 |        961 |
| NttManagerNoRateLimiting              |   18,169 |      6,407 |

Contract sizes with this feature:

| NttManager                                  |   23,651 |        925 |
| NttManagerNoRateLimiting                    |   18,205 |      6,371 |
| NttManagerWithPerChainTransceivers          |   22,804 |      1,772 |

bruce-riley · 2024-09-28T16:09:15Z

Would like to have a longer look at this next week, the bitmap and threshold stuff was a source of some pain in the original audit.

One general comment is I feel like it becomes a lot easier to misconfig with these changes. For example, in a 2 transceiver, threshold of 2 global setup, if only 1 transceiver is enabled for receiving from a given chain, but the deployer forgets to set the perChainThreshold for that chain, then messages won't get delivered. Maybe in enableRecvTransceiverForChain we need to check that perChainThreshold is <= set bits in the joint enabledTransceiverBitmap & enabledTransceiversForChain bitmap?

That would require them to set the per-chain threshold before enabling anything, right? Is that acceptable?

djb15 · 2024-09-28T19:47:59Z

Would like to have a longer look at this next week, the bitmap and threshold stuff was a source of some pain in the original audit.
One general comment is I feel like it becomes a lot easier to misconfig with these changes. For example, in a 2 transceiver, threshold of 2 global setup, if only 1 transceiver is enabled for receiving from a given chain, but the deployer forgets to set the perChainThreshold for that chain, then messages won't get delivered. Maybe in enableRecvTransceiverForChain we need to check that perChainThreshold is <= set bits in the joint enabledTransceiverBitmap & enabledTransceiversForChain bitmap?

That would require them to set the per-chain threshold before enabling anything, right? Is that acceptable?

I was thinking we could write perChainThreshold to 1 when we add the first per chain transceiver? I think we do something similar in the existing transceiver registry

banescusebi

I will continue looking a bit into the test files, but wanted to submit the comments so far.

evm/src/NttManager/NttManagerWithPerChainTransceivers.sol

evm/src/interfaces/IManagerBase.sol

evm/src/NttManager/NttManagerWithPerChainTransceivers.sol

evm/test/PerChainTransceivers.t.sol

bruce-riley · 2024-10-01T14:03:08Z

We've decided to not support falling back to the global defaults, and also switch to functions to get / set the bitmaps per chain, rather than doing enable/disable.

I'm converting this back to draft while I do the rework.

evm/README.md

djb15

Should setThreshold be overridden and revert?
More generally to the above bullet point, we should try to minimise unintended side effects of enabling/disabling transceivers globally/per-chain and also setting global/per-chain thresholds (global thresholds appear to be unused now, but worth being explicit)

evm/src/NttManager/NttManagerWithPerChainTransceivers.sol

banescusebi

Overall looks pretty solid this way. Just a few nits and questions from my side.

banescusebi · 2024-10-09T11:54:30Z

evm/src/NttManager/NttManagerWithPerChainTransceivers.sol

+    /// @notice The structure of a per-chain entry in the call to setTransceiversForChains.
+    struct SetTransceiversForChainEntry {
+        uint64 sendBitmap;
+        uint64 recvBitmap;


Genera question regarding this 64 transceiver limit: as more and more chains pop-up and are added to WH, will 64 bits be sufficient in the future?

banescusebi · 2024-10-09T12:44:03Z

evm/src/NttManager/NttManagerWithPerChainTransceivers.sol

    function _isSendTransceiverEnabledForChain(
        address transceiver,
        uint16 forChainId
    ) internal view override returns (bool) {
        uint64 bitmap = _getPerChainTransceiverBitmap(forChainId, SEND_TRANSCEIVER_BITMAP_SLOT);
        uint8 index = _getTransceiverInfosStorage()[transceiver].index;
-        return (bitmap & uint64(1 << index)) != 0;
+        return (bitmap & uint64(1 << index)) > 0;


Why this small change here?

This was me! != needs 2 opcodes vs > just needs 1, so we save (a small amount of) gas here. Plus it matches how we do it elsewhere in the codebase

evm/src/NttManager/NttManagerWithPerChainTransceivers.sol

banescusebi · 2024-10-09T13:00:56Z

evm/src/NttManager/NttManagerWithPerChainTransceivers.sol

+        uint64 enabledTransceiverBitmap = _getEnabledTransceiversBitmap();
+        uint64 enabledTransceiversForChain =
+            _getEnabledRecvTransceiversForChain(attInfo.sourceChainId);
+        return attInfo.attestedTransceivers & enabledTransceiverBitmap & enabledTransceiversForChain;


The _checkTransceiversInvariants() ensures that you can't remove a transceiver globally, when it's enabled for a chain. Therefore enabledTransceiversForChain is a subset of enabledTransceiverBitmap.

banescusebi · 2024-10-09T13:41:10Z

evm/src/NttManager/NttManagerWithPerChainTransceivers.sol

+    function _checkTransceiversInvariants() internal view override {
+        super._checkTransceiversInvariants();
+        _validateTransceivers(SEND_ENABLED_CHAINS_SLOT, SEND_TRANSCEIVER_BITMAP_SLOT);
+        _validateTransceivers(RECV_ENABLED_CHAINS_SLOT, RECV_TRANSCEIVER_BITMAP_SLOT);


This _checkTransceiversInvariants() seems quite heavy. Would it be worth doing an analysis at how many chains and how many transceivers per chain do the calling functions hit the block gas limit?

banescusebi · 2024-10-09T13:44:07Z

evm/test/PerChainTransceivers.t.sol


-        // Can't enable transceiver for an unknown address.
+        // Can't globally remove a transceiver when it's enabled for a chain.


Love this test.

banescusebi · 2024-10-09T13:51:07Z

evm/README.md

-The `NttManagerWithPerChainTransceivers` contract which inherits from `NttManagerNoRateLimiting` and allows configuring different transceivers and thresholds for each chain.
-You can configure a different set of send and receive transceivers for each chain. If you don't specifically enable any transceivers for a chain, then all transceivers will be used.
+The `NttManagerWithPerChainTransceivers` contract inherits from `NttManagerNoRateLimiting` and allows configuring different transceivers and thresholds for each chain.
+You can configure a different set of send and receive transceivers for each chain, as well as the receive threshold for the chain.


Something that might be worth adding to the README.md:

Even when only using per chain transceivers, one needs to register each transceiver using the setTransceiver() method inherited from ManagerBase. Note that this only allows registering up to 64 transceivers in total (to be used as send and/or receive transceivers).

evan-gray · 2024-10-11T15:22:10Z

Closing this PR given the following:

the implementation has become increasingly complex to make safe for upgrading NttManagers while maintaining the config and method identifiers
these changes push the contract over the size limit
we expect to rework the transceiver configuration in a future, breaking version

Therefore, despite the much appreciated effort and reviews, I do not recommend proceeding.

bruce-riley force-pushed the evm/per-chain-transceivers branch 4 times, most recently from 54ec5cd to 964a4de Compare September 16, 2024 21:13

bruce-riley force-pushed the evm/per-chain-transceivers branch 15 times, most recently from e442b2d to 6f7cfc4 Compare September 25, 2024 15:22

bruce-riley force-pushed the evm/per-chain-transceivers branch 10 times, most recently from 1270075 to 6eebeda Compare September 26, 2024 19:37

bruce-riley force-pushed the evm/per-chain-transceivers branch 2 times, most recently from 5aa1e47 to e6a1aca Compare September 29, 2024 16:25

banescusebi reviewed Sep 30, 2024

View reviewed changes

bruce-riley marked this pull request as draft October 1, 2024 14:03

bruce-riley force-pushed the evm/per-chain-transceivers branch from ff98457 to b936d9c Compare October 1, 2024 20:29

evan-gray reviewed Oct 1, 2024

View reviewed changes

evm/README.md Outdated Show resolved Hide resolved

evan-gray reviewed Oct 1, 2024

View reviewed changes

evm/README.md Outdated Show resolved Hide resolved

bruce-riley force-pushed the evm/per-chain-transceivers branch from b936d9c to a1fba4a Compare October 1, 2024 21:54

bruce-riley marked this pull request as ready for review October 2, 2024 19:37

bruce-riley requested review from evan-gray, djb15 and banescusebi October 2, 2024 19:38

djb15 reviewed Oct 3, 2024

View reviewed changes

kcsongor and others added 9 commits October 8, 2024 07:57

evm: add support for per-chain enabled transceivers

7cdbbab

EVM/Add setter and tests for per-chain thresholds

7ba3b74

EVM/ Add support for per-chain transceivers

c328399

EVM: Rework per-chain transceivers

e9a2d46

EVM: Per chain transceiver code cleanup

f4b2d76

EVM: Flip contract hierarchy for NttManagerWithPerChainTransceivers

f6f994d

EVM: NttManagerWithPerChainTransceivers review rework

f218c1e

EVM: Per-Chain Transceivers no defaults

c5b58b3

EVM: Remove NttManagerWithPerChainTransceivers from IManagerBase

dd0400d

bruce-riley force-pushed the evm/per-chain-transceivers branch from a3f7772 to dd0400d Compare October 8, 2024 13:57

EVM: per-chain transceiver code review rework

0bff03c

banescusebi reviewed Oct 9, 2024

View reviewed changes

evan-gray closed this Oct 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

evm: per chain transceivers #517

evm: per chain transceivers #517

bruce-riley commented Sep 12, 2024 •

edited

Loading

bruce-riley commented Sep 25, 2024 •

edited

Loading

bruce-riley commented Sep 28, 2024

djb15 commented Sep 28, 2024

banescusebi left a comment

bruce-riley commented Oct 1, 2024

djb15 left a comment

banescusebi left a comment

banescusebi Oct 9, 2024

banescusebi Oct 9, 2024

djb15 Oct 9, 2024

banescusebi Oct 9, 2024

banescusebi Oct 9, 2024

banescusebi Oct 9, 2024

banescusebi Oct 9, 2024

evan-gray commented Oct 11, 2024 •

edited

Loading


		// Can't enable transceiver for an unknown address.
		// Can't globally remove a transceiver when it's enabled for a chain.

evm: per chain transceivers #517

evm: per chain transceivers #517

Conversation

bruce-riley commented Sep 12, 2024 • edited Loading

bruce-riley commented Sep 25, 2024 • edited Loading

bruce-riley commented Sep 28, 2024

djb15 commented Sep 28, 2024

banescusebi left a comment

Choose a reason for hiding this comment

bruce-riley commented Oct 1, 2024

djb15 left a comment

Choose a reason for hiding this comment

banescusebi left a comment

Choose a reason for hiding this comment

banescusebi Oct 9, 2024

Choose a reason for hiding this comment

banescusebi Oct 9, 2024

Choose a reason for hiding this comment

djb15 Oct 9, 2024

Choose a reason for hiding this comment

banescusebi Oct 9, 2024

Choose a reason for hiding this comment

banescusebi Oct 9, 2024

Choose a reason for hiding this comment

banescusebi Oct 9, 2024

Choose a reason for hiding this comment

banescusebi Oct 9, 2024

Choose a reason for hiding this comment

evan-gray commented Oct 11, 2024 • edited Loading

bruce-riley commented Sep 12, 2024 •

edited

Loading

bruce-riley commented Sep 25, 2024 •

edited

Loading

evan-gray commented Oct 11, 2024 •

edited

Loading