Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multi-chain reputation #1132

Closed
wants to merge 30 commits into from
Closed

Multi-chain reputation #1132

wants to merge 30 commits into from

Conversation

area
Copy link
Member

@area area commented Apr 17, 2023

Goal

There is a desire for Colony to be deployed to multiple chains. However, the idea of requiring reputation mining to be deployed to multiple chains at once is unpalatable for a variety of reasons - needing the token bridged to all relevant chains, needing to stake separately on all chains, having to monitor all chains to name a few.

We therefore aim for a simpler extension of reputation mining; mining remains on a single chain, but where necessary (e.g. adding new log entries to the mining log), we use bridges to send transactions from other chains to the chain where mining is taking place.

Implementation

The main change to existing functions is the function that adds a reputation update to the log. On the mining chain, it behaves as it currently does, and on any other chain it sends a transaction to the bridge. That transaction, once taken across the bridge (by whatever mechanism the bridge uses) will add the log entry to the log. Similarly, on skill creation, if not on the mining chain, a bridging transaction is sent in addition to all the usual behaviour, which when bridged to the mining chain registers the skill as created, and therefore eligible for reputation to be earned in.

The support for bridging transactions is intended to be agnostic to the actual details of the bridges themselves, as we certainly cannot guarantee that all bridges will follow the interface that our mock bridge does. When adding details of bridges on either the mining chain or another chain, we provide the address of the bridge, the chainId of the chain on the other side of the bridge, and some combination of pairs of bytes representing the three types of bridge transactions that there are:

  • Bridging a reputation update (intended to go from non-mining chain to mining chain)
  • Bridging skill creation (intended to go from non-mining chain to mining chain)
  • Bridging a new reputation state (intended to go from mining chain to non-mining chain)

When constructing a bridging transaction, the contracts construct the transaction that needs to be called on the other side of the bridge, and then concatenates the 'before' and 'after' bytes arrays that were provided when registering the bridge. Not all of these arrays will be provided for any given bridge registration, as each colonyNetwork instance will only call a bridge in at most two of these three ways, depending on whether it is on the mining chain or not. A functional deployment will require all of these to be set between the two colony network deployments on the two chains.

Where appropriate, functions have been marked with a new onlyMiningChain modifier to try and make sure they're not accidentally called. Functions only intended to be called by bridges check that they are coming from a known bridge.

We expect only one bridge to be registered on any non-mining chain - the address of a bridge back to the mining chain; this address is stored in the miningBridgeAddress storage slot. This slot is not intended to be used on the mining chain, where many bridges will be registered (i.e. all the bridges to all the non-mining chains).

In principle, I believe this scheme also allows the possibility of 'middlemen' - if a bridge does not exist between the mining chain and the desired foreign chain, then a bridge can be registered for the desired chain but is, actually, a bridge to an intermediary chain. The transactions constructed could be such that they call another bridge on that chain directly, which is a bridge to a desired foreign chain. Obviously, gas fees, and having to make sure such transactions are executed all the way along, but probably lower effort to write a service that does this rather than deploying and maintaining our own bridges for any and all chain combinations.

Assumptions

Note that inherently, here, we trust bridges to be honest. If a bridge is compromised, and starts sending 'bridged' transactions arbitrarily:

  • Arbitrary reputation values will be able to be added/removed to colonies on the chain corresponding to the compromised bridge.
  • The skill trees on the two chains will get out of sync

Notably, the issues (should be...) confined to colonies on the chain corresponding to the compromised bridge. In the case that a 'bad' reputation state is sent to the colonyNetwork on a chain with a compromised bridge, the 'right' state can be emitted again and bridged.

Design Decisions

skillIds on non-mining chains start at a value of chainId << 128. This makes skill ids unique across chains - a requirement for reputation mining. I believe this also means there is no transitional period required - once deployed, and bridged skills and transactions start appearing, they should just be incorporated in to the log. I am a little wary of the fact that chainId is by definition a uint256 whereas we're essentially truncating it in to a uint128 here. I am therefore very tempted to hash the chainId and use (part of) the resulting hash as the first 'half' of the skillId. This is better than hashing both the skill count and the chainId together, as skillIds remain monotonically increasing per-chain allowing checks for whether a skill exists and not requiring a storage slot to be set per-skill even if it's not otherwise necessary.

Note that everything to do with global skills is limited to the mining chain only, and is ultimately planned to be deprecated there too - we do not need universal global skills, and so there is no requirement to bridge those that already exist from the mining chain, nor a requirement for their creation to be supported on other chains.

The intention is for the system to be relatively resilient to bridges behaving in unexpected ways. For example, a bridge might not bridge a transaction, or might bridge transactions out of order. Skills must be added in order (on a per-chain basis), which could cause issues. As a result, if a skill is attempted to be added out-of-order on the mining chain, it's stored and can be added later. Once all previous skills have been added, either via re-bridging or via this 'pending' mechanism, the skill can be added as normal with an appropriate transaction on the mining chain.

The current reputation state can be bridged to any chain at any time on demand. It is not automatically bridged on an update for gas reasons; if such behaviour is desired, it should be implemented via a standalone service.

Currently, if an entry in the reputation log is bridged for a skill that does not currently exist, it is dropped. On reflection, I think a better behaviour would be a similar scheme for skill addition, where such an emission would be stored and could be added to the log once the skill has been created, with the value decayed by an appropriate amount based on the time between the original emission and the addition to the log.

In general, all these failure cases are open to debate and should arguably go up-stream to a conversation involving some or all of Jack / Aron / Daniel / Arren

@kronosapiens
Copy link
Member

This is better than hashing both the skill count and the chainId together, as chainIds remain monotonically increasing per-chain allowing checks for whether a skill exists and not requiring a storage slot to be set per-skill even if it's not otherwise necessary.

Assuming you mean skillIds are monotonically increasing?

@area
Copy link
Member Author

area commented Apr 18, 2023

I do, updated.

contracts/colonyNetwork/ColonyNetwork.sol Outdated Show resolved Hide resolved
contracts/colonyNetwork/ColonyNetwork.sol Outdated Show resolved Hide resolved
contracts/colonyNetwork/ColonyNetwork.sol Outdated Show resolved Hide resolved
contracts/colonyNetwork/ColonyNetwork.sol Outdated Show resolved Hide resolved
contracts/colonyNetwork/ColonyNetwork.sol Outdated Show resolved Hide resolved
truffle.js Show resolved Hide resolved
test-smoke/colony-storage-consistent.js Outdated Show resolved Hide resolved
test/cross-chain/cross-chain.js Show resolved Hide resolved
test/cross-chain/cross-chain.js Outdated Show resolved Hide resolved
@area area force-pushed the feat/multichain-reputation branch 9 times, most recently from fdd2db0 to 5aed2f0 Compare May 5, 2023 14:06
@area area mentioned this pull request May 26, 2023
@area area force-pushed the feat/multichain-reputation branch from e1754ed to 8e117b1 Compare May 31, 2023 08:19
.solcover.crosschain.js Outdated Show resolved Hide resolved
contracts/colony/Colony.sol Outdated Show resolved Hide resolved
contracts/colony/Colony.sol Outdated Show resolved Hide resolved
contracts/colonyNetwork/ColonyNetwork.sol Outdated Show resolved Hide resolved
contracts/colonyNetwork/ColonyNetwork.sol Outdated Show resolved Hide resolved
test/cross-chain/cross-chain.js Outdated Show resolved Hide resolved
test/cross-chain/cross-chain.js Outdated Show resolved Hide resolved
test/cross-chain/cross-chain.js Show resolved Hide resolved
test/cross-chain/cross-chain.js Outdated Show resolved Hide resolved
test/cross-chain/cross-chain.js Outdated Show resolved Hide resolved
Copy link
Member

@kronosapiens kronosapiens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Second review

@area area force-pushed the feat/multichain-reputation branch 2 times, most recently from e8dafbd to 54020ca Compare June 10, 2023 20:50
@kronosapiens kronosapiens force-pushed the feat/multichain-reputation branch 9 times, most recently from 31ed8e2 to 01f14fc Compare June 14, 2023 15:26
@kronosapiens
Copy link
Member

@area I've addressed all my own comments. There are a few failing tests but they were failing on your last commit so I'm assuming you haven't gotten to them yet. Once you've gotten everything to your liking and taken the PR off draft I'll give it one more review and then I think we should be ready to merge.

@area
Copy link
Member Author

area commented May 2, 2024

The reworked version of this ended up in #1216 which was included with the merge of #1237.

@area area closed this May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants