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

Aragon Court POC implementation #6

Merged
merged 27 commits into from Mar 25, 2019
Merged

Aragon Court POC implementation #6

merged 27 commits into from Mar 25, 2019

Conversation

@izqui
Copy link
Member

izqui commented Feb 22, 2019

  • Staking: ERC900
  • Court state machine: heartbeat
  • Juror token activation and deactivation
  • Dispute state machine
  • Commit-reveal voting
  • Fee and juror token distribution

Resolves #2
Fixes #20

@sohkai sohkai added this to the A1 Sprint: 4.2 milestone Feb 22, 2019
Copy link
Member

bingen left a comment

Looking good so far.

contracts/Court.sol Outdated Show resolved Hide resolved
contracts/Court.sol Outdated Show resolved Hide resolved
contracts/Court.sol Outdated Show resolved Hide resolved
}
for (uint256 j = 0; j < egressLength; j++) {
address jurorEgress = _incomingTerm.egressQueue[j];
sumTree.set(accounts[jurorEgress].sumTreeId, 0);

This comment has been minimized.

Copy link
@bingen

bingen Feb 25, 2019

Member

We should make sure that first juror (sumTreeId = 0) is not set by accident. One simple option would be to insert a fake juror in the tree with amount zero, and check that id > 0, at least when trying to update amount to a positive value (like line 361 below)

This comment has been minimized.

Copy link
@izqui

izqui Mar 1, 2019

Author Member

Good idea, lets use 0 as an empty juror.

This comment has been minimized.

Copy link
@izqui

This comment has been minimized.

Copy link
@bingen

bingen Mar 1, 2019

Member

Wouldn't you do that for updatesQueue below too? (L366 now). I think it's even more important than here, because in this one you were setting anyway to 0 (the value it already had). Actually we could even revert, as there's something weird going on if we try to update the reserved 0 position.
If for some reason it's impossible that updatesQueue can be accidentally set for juror 0, and we are completely sure about it, I would at least put a comment explaining it.

import "@aragon/os/contracts/common/SafeERC20.sol";


contract Court is ERC900, ApproveAndCallFallBack {

This comment has been minimized.

Copy link
@bingen

bingen Feb 25, 2019

Member

Have you considered using the Staking app? I see upsides and downsides on every option, but I'm curious about your reasoning here. What I like better of having it embedded here is that makes things easier as jurors don't need to create a lock and pass the lockId, etc. Was that the reason?

This comment has been minimized.

Copy link
@izqui

izqui Mar 1, 2019

Author Member

I did consider it but this needs the most basic version of staking without checkpointing which is trivial to implement, we could migrate it to use Staking at some point if it makes the Court code simpler and the gas overhead of the added calls is not to much.

}

function canTransitionTerm() public view returns (bool) {
return neededTermTransitions() >= 1;

This comment has been minimized.

Copy link
@bingen

bingen Feb 25, 2019

Member

I think this is simpler and slightly cheaper:

return time() >= terms[term].startTime + termDuration;

This comment has been minimized.

Copy link
@izqui

izqui Mar 1, 2019

Author Member

It would, although really doesn't make a huge difference as it is just some extra arithmetic (a division) that is really cheap.

This comment has been minimized.

Copy link
@izqui

izqui Mar 1, 2019

Author Member

Opted to just have the same logic to check how many terms need to be transitioned and whether just 1 needs transitioning

contracts/Court.sol Outdated Show resolved Hide resolved
@izqui izqui changed the base branch from sortition to master Mar 1, 2019
@izqui izqui force-pushed the spike branch from d93e37e to c405771 Mar 1, 2019
@sohkai sohkai modified the milestones: A1 Sprint: 4.2, A1 Sprint: 4.3 Mar 4, 2019
@luisivan luisivan mentioned this pull request Mar 4, 2019
Closed
0 of 4 tasks complete
contracts/standards/arbitration/Arbitrable.sol Outdated Show resolved Hide resolved
* @param _disputeId Id of the dispute in the Court contract.
* @param _ruling Ruling given by the arbitrator. Note that 0 is reserved for "Not able/wanting to make a decision".
*/
function _executeRuling(uint256 _disputeId, uint256 _ruling) internal;

This comment has been minimized.

Copy link
@bingen

bingen Mar 5, 2019

Member

This is a TODO, right? Or is it going to be abstract too?

This comment has been minimized.

Copy link
@izqui

izqui Mar 9, 2019

Author Member

I think it makes sense for Agreement to be an abstract contract and require contracts that inherit from it to implement the function.

This comment has been minimized.

Copy link
@bingen

bingen Mar 22, 2019

Member

But this is already in Arbitrable. Do we need to repeat it here?

This comment has been minimized.

Copy link
@izqui

izqui Mar 23, 2019

Author Member

We could actually implement the management of stake locks in this contract: #13



contract Agreement is Arbitrable /* AragonApp/Trigger */ {
address[] parties;

This comment has been minimized.

Copy link
@bingen

bingen Mar 5, 2019

Member

This should probably be 2, proposer and challenger, and challenger won't be known in advance. Let's discuss it.

uint256 baseKey = BASE_KEY;

for (uint256 i = 0; i < n; i++) {
uint256 random = sum % (seed * i); // intended overflow

This comment has been minimized.

Copy link
@bingen

bingen Mar 5, 2019

Member

This won't be uniform. Ideally you would want to have a sequence that goes over all vales [0,n) uniformly. Once seed * i surpasses sum, random will always be sum, so if seed or i are big it ill be quite biased.
If you try to do the opposite ((seed * i) % sum) it wouldn't be uniform unless sum and seed are co-primes. for instance, imagine that sum = 12 and seed = 8, then you would have: 8, 16, 24, 32, 40,... which would turn into 8, 4, 0, 8, 4, .... I think this last option would work if we cold ensure that the seed that we get is prime.

This comment has been minimized.

Copy link
@izqui

izqui Mar 9, 2019

Author Member

As we chatted offline it probably makes sense to ensure a more uniform random distribution with random = keccak256(seed, i) % sum. This will add a gas overhead of 42 gas (as we are hashing 2 words) which I don't think it is too bad, although it feels like a hack and would love to find a way to do this securely using simpler math than hashing.

if (_token == jurorToken) {
if (account.state == AccountState.Juror) {
require(isJurorBalanceUnlocked(addr), ERROR_JUROR_TOKENS_AT_STAKE);
account.state = AccountState.PastJuror;

This comment has been minimized.

Copy link
@bingen

bingen Mar 6, 2019

Member

I don't fully get how this juror flow works, in particular the PastJuror state. To activate it needs to be in NotJuror state, and to deactivate in Juror state, so would that mean that once the juror withdraws it can not act as a juror anymore?

This comment has been minimized.

Copy link
@izqui

izqui Mar 9, 2019

Author Member

Yes. It simplifies a lot of logic, if someone wants to become a juror again, they should do it from another account.

This comment has been minimized.

Copy link
@bingen

bingen Mar 22, 2019

Member

Sounds acceptable for a first version, but quite a bad user experience. If for some reason a juror activates and deactivates often, it would be very inconvenient. Even worse if some day we add identity to the mix, which could be interesting.

This comment has been minimized.

Copy link
@izqui

izqui Mar 23, 2019

Author Member

Agreed. Created an issue to explore how feasible it would be to enable this: #12

address public governor; // TODO: consider using aOS' ACL
uint64 public jurorCooldownTerms;
uint256 public jurorActivationDust;
uint256 public maxAppeals = 5;

This comment has been minimized.

Copy link
@bingen

bingen Mar 6, 2019

Member

It seems it's not used yet, but why this 5? Is it because of the max number of jurors due to gas constrains?

This comment has been minimized.

Copy link
@izqui

izqui Mar 9, 2019

Author Member

Magic number coming from the Kleros implementation, will remove and properly configure it when implementing appeals. The upper limit for appeals should take into account the maximum amount of time it would take to resolve a dispute (social constrain, we don't want disputes to be pending for a year) and the maximum number of jurors that we can draft within a term (gas constrain, specially if we don't implement drafting in multiple transactions).

term += 1;
emit NewTerm(term, heartbeatSender);

if (_termTransitions > 0 && canTransitionTerm()) {

This comment has been minimized.

Copy link
@bingen

bingen Mar 6, 2019

Member

So I understand here that:

  • If you want to transition only one term you must call the function with _termTransitions set to zero. Not a big deal, but sounds kind of counter-intuitive to me.
  • If more than one transitions are requested, it will revert if the first one can not go through, but if it's the second one or later which can't be done, it will do as many as possible and return successfully. If that's the intended behavior it's fine for me, but once function headers are added we should put a comment there explaining it.

Are those assumptions correct?

This comment has been minimized.

Copy link
@bingen

bingen Mar 6, 2019

Member

About the second one, I think it would be a problem for ensureTerm modifier, because heartbeat could return without having transitioned all needed terms. I guess it would be better to just remove that && canTransitionTerm() condition and leave the function revert on the recursive call.

This comment has been minimized.

Copy link
@bingen

bingen Mar 6, 2019

Member

Another comment is that _termTransitions is limited to the max recursion stack depth, it may be good to check it in advance, to avoid accidentally wasting gas (or at least warn it in a comment in the function header).
I was wondering if it could be increased by converting the recursion to a loop, but I guess it's not worth it as the current limit would be around ~42 days (if I'm not wrong), which sounds reasonable.

This comment has been minimized.

Copy link
@izqui

izqui Mar 9, 2019

Author Member

If you want to transition only one term you must call the function with _termTransitions set to zero.

This is counterintuitive, it should definitely be the maximum number of transitions you want to do. We should do this change: https://github.com/aragonone/aragon-court/pull/6/files#r263994971

If more than one transitions are requested, it will revert if the first one can not go through, but if it's the second one or later which can't be done, it will do as many as possible and return successfully

My rationale here is to avoid reverting some gas heavy computation which has already been paid for and there is no reason to undo (even if the caller wanted to perform more transitions). heartbeat() callers should however calculate off-chain how many transitions they can do within the block gas limit.

is limited to the max recursion stack depth

How is this an issue if these are all internal jumps and not external calls (which AFAIK is when the stack depth error applies)?

This comment has been minimized.

Copy link
@bingen

bingen Mar 22, 2019

Member

My rationale here is to avoid reverting some gas heavy computation which has already been paid for and there is no reason to undo (even if the caller wanted to perform more transitions). heartbeat() callers should however calculate off-chain how many transitions they can do within the block gas limit.

Yes, that makes sense, but what about ensureTerm? That modifier wouldn't be actually ensuring it if for some reason fails after the second one.

How is this an issue if these are all internal jumps and not external calls (which AFAIK is when the stack depth error applies)?

Oh, good point. This is one of those unlimited resources assumptions that feel quite counter-intuitive to me, haha.
Besides, it seems that that number was outdated too:
https://ethereum.stackexchange.com/a/9399/9205

This comment has been minimized.

Copy link
@izqui

izqui Mar 23, 2019

Author Member

Because MODIFIER_ALLOWED_TERM_TRANSITIONS = 1, the ensureTerm modifier will revert if more than one transition is required. I am assuming that heartbeat() will be called by bots, but in case a random transaction is mined before a heartbeat() if it was sent with enough gas to cover a potential term transition, the transaction won't revert.

}

function canTransitionTerm() public view returns (bool) {
return neededTermTransitions() >= 1;

This comment has been minimized.

Copy link
@bingen

bingen Mar 6, 2019

Member

This is only checking time condition, but what about if a term has "content" in it? For instance if has queues to be managed, or appeals or disputes to be resolved? What would happen in such cases if several terms are transitioned in a row?

This comment has been minimized.

Copy link
@izqui

izqui Mar 9, 2019

Author Member

if has queues to be managed

They get processed in the heartbeat, so it is not an issue

or appeals or disputes to be resolved

Disputes that have an adjudication round pegged to a particular term have the responsibility of executing their draft (and there is a fee for whoever does that) within that time frame. If they miss it, I believe it is better to reschedule these rounds rather than blocking the court.


// TODO: actually draft jurors
if (draftTerm.randomness == 0) {
// the blockhash could be 0 if the first dispute draft happens 256 blocks after the term starts

This comment has been minimized.

Copy link
@bingen

bingen Mar 6, 2019

Member

So, is this a "TODO"? What's the idea? Rely on EIP-210? It doesn't seem to fix the problem for us either.

This comment has been minimized.

Copy link
@izqui

izqui Mar 9, 2019

Author Member

We don't know when EIP210 will land, so we can't really rely on it.

If the term duration is set to one hour (a reasonable time IMO for multiple reasons), then it is very unlikely that a term that needs randomness will take more than 256 blocks to request it if the drafting fee is properly set (as there should be a race to earn the fee ASAP).

In case that it really takes more than 256 blocks, there are two options: either reschedule all adjudication rounds that are drafted in this term to a future term (as we will need to do if the drafting function isn't called within the term) or just modify the term's randomness block number to the latest block hash that we can get (currentBlock - 256). Another alternative would be to use 0 as the randomness seed if this happens, which doesn't really give an advantage to anyone since we will be hashing it with the disputeId.

// TODO: actually draft jurors
if (draftTerm.randomness == 0) {
// the blockhash could be 0 if the first dispute draft happens 256 blocks after the term starts
draftTerm.randomness = uint256(block.blockhash(draftTerm.randomnessBN));

This comment has been minimized.

Copy link
@bingen

bingen Mar 6, 2019

Member

Could it happen that this function is called in the same block as heartbeat? Then we wouldn't have randomness seed. (I don't know what happens when you call that opcode with an out of bounds block number)

This comment has been minimized.

Copy link
@izqui

izqui Mar 9, 2019

Author Member

Good catch, we should protect against this. I am pretty sure it just returns bytes32(0)


function draftAdjudicationRound(uint256 _disputeId)
public
ensureTerm

This comment has been minimized.

Copy link
@izqui

izqui Mar 9, 2019

Author Member

This will transition the term if necessary, but this function should revert if the term is transitioned in the modifier, as the randomness of the term won't be available until the next block as @bingen points out here:
https://github.com/aragonone/aragon-court/pull/6/files#r263090838

term += 1;
emit NewTerm(term, heartbeatSender);

if (_termTransitions > 0 && canTransitionTerm()) {

This comment has been minimized.

Copy link
@izqui

izqui Mar 9, 2019

Author Member
Suggested change
if (_termTransitions > 0 && canTransitionTerm()) {
if (_termTransitions.sub(1) > 0 && canTransitionTerm()) {
Copy link
Member

bingen left a comment

So far mostly minor things, but I still have to dig deeper into it. My main concern right now is that this is huge, and I wonder if it wouldn't be better to split things up, like we talked about Staking, but more generally... not sure.

term += 1;
emit NewTerm(term, heartbeatSender);

if (_termTransitions > 0 && canTransitionTerm()) {

This comment has been minimized.

Copy link
@bingen

bingen Mar 22, 2019

Member

My rationale here is to avoid reverting some gas heavy computation which has already been paid for and there is no reason to undo (even if the caller wanted to perform more transitions). heartbeat() callers should however calculate off-chain how many transitions they can do within the block gas limit.

Yes, that makes sense, but what about ensureTerm? That modifier wouldn't be actually ensuring it if for some reason fails after the second one.

How is this an issue if these are all internal jumps and not external calls (which AFAIK is when the stack depth error applies)?

Oh, good point. This is one of those unlimited resources assumptions that feel quite counter-intuitive to me, haha.
Besides, it seems that that number was outdated too:
https://ethereum.stackexchange.com/a/9399/9205

{
checkVote(_disputeId, _roundId, _draftId, _leakedRuling, _salt);

uint8 ruling = uint8(Ruling.RefusedRuling);

This comment has been minimized.

Copy link
@bingen

bingen Mar 22, 2019

Member

Shouldn't this be Ruling.Missing? Refusing is actually an option and this cast vote could become decisive.

contracts/Court.sol Outdated Show resolved Hide resolved
contracts/Court.sol Outdated Show resolved Hide resolved
contracts/Court.sol Outdated Show resolved Hide resolved
CourtConfig[] public courtConfigs;

// Court state
uint64 public term;

This comment has been minimized.

Copy link
@bingen

bingen Mar 22, 2019

Member

Maybe we could call this currentTermId.

Dispute storage dispute = disputes[_disputeId];
AdjudicationRound storage currentRound = dispute.rounds[_roundId];

uint64 appealJurorNumber = 2 * currentRound.jurorNumber + 1; // J' = 2J + 1

This comment has been minimized.

Copy link
@bingen

bingen Mar 22, 2019

Member

We should check that we are not surpassing the maximum, right?

This comment has been minimized.

Copy link
@izqui

izqui Mar 25, 2019

Author Member

Yes, created issue to track this: #18

contracts/Court.sol Show resolved Hide resolved
contracts/Court.sol Outdated Show resolved Hide resolved
contracts/Court.sol Show resolved Hide resolved
@izqui izqui changed the title Aragon Court implementation Aragon Court POC implementation Mar 25, 2019
@izqui izqui marked this pull request as ready for review Mar 25, 2019
@izqui izqui merged commit 5201088 into master Mar 25, 2019
@izqui izqui deleted the spike branch Mar 25, 2019
@bingen bingen mentioned this pull request May 13, 2019
@izqui izqui modified the milestones: A1 Sprint: 4.3, Freeze #1 Oct 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.