Parameterize constants

[guidanoli]

Parameterizes the following constants from ArbitrationConstants:

• MATCH_EFFORT
• MAX_ALLOWANCE
• LEVELS
• log2step[]
• height[]

Closes #85

[guidanoli]

I tried adding each parameter separately as constructor arguments but ended in the stack-too-deep problem. Turning Via-IR ON would break the tests for some reason. So, I decided to pass just the address of the factory contract so that it could be queried later for the parameters through the ITournamentParameters interface.

[guidanoli]

I suspect this code block wasn't doing anything. So, I moved it before the assignment to allowance, and created a min function in the Time library to simplify code.

[GCdePaula]

Nice catch. It's possible we can just remove it though. Let's discuss this more.

[guidanoli]

It's a safeguard. If we can prove that _allowance will always be less than or equal to the max allowance, then we can remove this explicit cap.

[GCdePaula]

My reasoning is that if we are sure the clock regeneration is capped, then the max of two capped things will also be capped.

We have to verify if the regeneration is capped.

[guidanoli]

Maybe an array this big is so unlikely that this line could be converted into an assertion.

[GCdePaula]

I see that now tournaments receive both an IFactory and an ITournamentParameters. But aren't these the same address? Couldn't we pass it once?

Like, maybe we should make IFactory inherit from ITournamentParameters, so that we pass only one address (instead of the same address twice).

This conceptually makes sense, since a factory would also specify what are the tournament parameters. We should eventually also make it specify the step.

===

Another approach. First, let's keep the factory defining the parameters as described in the previous paragraph.

This specification is in the form of two structs: one (A) that tells you the number of levels and an array of pairs that tell the log2step and height, and another (B) that tells the time parameters. These are constant. Maybe we should create a third struct C that contains A * B.

The approach is as follows: instead of passing an address where these informations are queryable, could we pass down C?

===

So two approaches, different than what's on the PR.

The first approach is similar to the current one, but the parameters and the factory are unified. Like, they already are at the implementation level (I think), but not at the interface level. This forces you to pass two addresses (with the same value), when we could pass a single address.

The second approach passes down an immutable struct, instead of an interface that once queried returns these constants.

[GCdePaula]

I think this library has a different purpose now.

As such, I think the library name inside CanonicalConstants.sol should be called CanonicalConstants and should perhaps be inside the script/ directory.

[guidanoli]

I just noticed we are still using CanonicalConstants under src/tournament, namely the constants LOG2_UARCH_SPAN and LOG2_INPUT_SPAN. Should those be parameterized as well?

[guidanoli]

Also, I forgot a MAX_ALLOWANCE under src/tournament/libs/Clock.sol but I'm patching this right now.

[GCdePaula]

Hmmm, they should be part of meta-step. This is a "harder" kind of constant, which is really part of the code. It doesn't make a lot of sense to be parametrized.

I think for now we can hardcode them on the leaf tournament. In the future, when we extract the step to the factory, we should put them there. At some point we could make configuring it easier, but I don't think these are values that we need to change a lot.

New releases of the machine could change these parameters. I don't think making them configurable helps our tests .

[GCdePaula]

Maybe this will break a lot of things, but...

Could it make sense for levels to decrement instead of increment? Maybe @stephenctw can help here.

The idea would be to have level 0 be the leaf tournament, and level LEVELS be the root. Then this check would be _level == 1, and we wouldn't need to pass down the LEVELS constant.

Don't know, just something that came to mind. I prefer not to break things, so maybe just ignore what I said.

[guidanoli]

I think it makes perfect sense to have a counter be decremented until a constant value (e.g., 0 or 1) instead of being incremented until a variable, because it allows us to remove such variable. However, I don't know the impact this would have on the PRT contracts, since a tournament to be a leaf and/or a root seem to be orthogonal properties, such that we have the four concrete combinations.

[GCdePaula]

Yeah, also other annoying changes. We'd have to reverse the order in the array of height+log2step, and even worse all the cascading changes offchain.

Let's consider this again later.

[GCdePaula]

Nice catch. It's possible we can just remove it though. Let's discuss this more.

[GCdePaula]

I'm thinking if we should create a struct of log2step * height, and have a single array of that.

[guidanoli]

We could, but it would take up more storage slots. Solidity reserves one entire storage slot for each (uint64,uint64) tuple (while it could, in theory, pack two into the same storage slot). Meanwhile, it tightly packs four uint64 values into the same storage slot. So, storing log2step and height as two separate arrays uses asymptotically half as many storage slots.

[GCdePaula]

I'm not super familiar with Solidity packing. I'm very confused about all this. Here are a bunch of questions.

Does all of this matter if it's immutable? We're using dynamic arrays. Is there really packing of dynamic arrays in storage? Can we actually make dynamic arrays immutable?

I believe static arrays and structs in memory are padded to 32 bytes, but both are packed when in storage. Like, I don't know how much space (uint64, uint64)[] would take in storage when compared with (uint64[], uint64[]). Could the second be worse because we'd be storing the dynamic array's size twice?

My conclusions are:

• I don't think it makes a difference in memory since it's not packed.
• Even if it did, I'm not worried about memory since the size is 3.
• In storage, since the array is dynamic, is it really tightly packed in either alternative?
• It could even be worse to have two dynamic arrays because we'd be storing size twice.

[guidanoli]

Is there really packing of dynamic arrays in storage?

There is. For example, uint64[] arrays store four elements per slot.

Can we actually make dynamic arrays immutable?

As of now, Solidity doesn't support immutable dynamic arrays.

Could the second be worse because we'd be storing the dynamic array's size twice?

So, being $n$ the number of levels of tournament, (uint64, uint64)[] uses $\left( 1 + n \right)$ storage slots, whereas (uint64[],uint64[]) uses $\left[ 2 \cdot \left( 1 + \left\lceil \frac{n}{4} \right\rceil \right) \right]$ storage slots. Here are some numbers:

$n$	Array of structs	Struct of arrays
1	2	4
2	3	4
3	4	4
4	5	4

So, for $n<3$, the first option uses less storage slots. For $n=3$, both use the same number of storage slots. And for $n>3$, the second option uses less storage slots.

In storage, since the array is dynamic, is it really tightly packed in either alternative?

Only if the array elements are of value types like uint64 but not tuples like (uint64,uint64).

[GCdePaula]

Great analysis! This cleared up my confusion.

I guess the conclusion is that gas-wise it's pretty much irrelevant, we can choose whatever we prefer since it won't impact costs?

[guidanoli]

Actually, gas-wise your solution is better because it needs to read only one array from storage when instantiating a tournament, instead of two. Also, we don't need to check if the array lengths match at construction time. Here is the gas diff:

Contract	Function	Min	Avg	Max
MiddleTournament	sealInnerMatchAndCreateInnerTournament	-3950 (-0.10%)	-3950 (-0.10%)	-3950 (-0.10%)
MultiLevelTournamentFactory				-17324 (-2.97%)
TopTournament	sealInnerMatchAndCreateInnerTournament	-3950 (-0.22%)	-3950 (-0.21%)	-3950 (-0.21%)

[GCdePaula]

I think we should maybe create a struct with this pair.

[guidanoli]

Doing so would increase the gas cost of the joinTournament function. Here is the diff:

diff --git a/prt/contracts/src/tournament/abstracts/Tournament.sol b/prt/contracts/src/tournament/abstracts/Tournament.sol
index 9d27147..15780d6 100644
--- a/prt/contracts/src/tournament/abstracts/Tournament.sol
+++ b/prt/contracts/src/tournament/abstracts/Tournament.sol
@@ -348,8 +348,10 @@ abstract contract Tournament {
                 _rootHash,
                 _leftNode,
                 _rightNode,
-                log2step,
-                height
+                Match.Constants({
+                    log2step: log2step,
+                    height: height
+                })
             );
 
             matches[_matchId] = _matchState;
diff --git a/prt/contracts/src/tournament/libs/Match.sol b/prt/contracts/src/tournament/libs/Match.sol
index 5236a96..b1c58ba 100644
--- a/prt/contracts/src/tournament/libs/Match.sol
+++ b/prt/contracts/src/tournament/libs/Match.sol
@@ -74,13 +74,20 @@ library Match {
         uint64 height; // constant
     }
 
+    //
+    // Constants
+    //
+    struct Constants {
+        uint64 log2step;
+        uint64 height;
+    }
+
     function createMatch(
         Tree.Node one,
         Tree.Node two,
         Tree.Node leftNodeOfTwo,
         Tree.Node rightNodeOfTwo,
-        uint64 log2step,
-        uint64 height
+        Constants memory constants
     ) internal pure returns (IdHash, State memory) {
         assert(two.verify(leftNodeOfTwo, rightNodeOfTwo));
 
@@ -91,9 +98,9 @@ library Match {
             leftNode: leftNodeOfTwo,
             rightNode: rightNodeOfTwo,
             runningLeafPosition: 0,
-            currentHeight: height,
-            log2step: log2step,
-            height: height
+            currentHeight: constants.height,
+            log2step: constants.log2step,
+            height: constants.height
         });
 
         return (matchId.hashFromId(), state);

[GCdePaula]

I don't feel strongly about either alternative. My 2c is that if the gas difference is small, we should go for what we prefer (I don't have a strong preference for either).

[guidanoli]

The approach is as follows: instead of passing an address where these informations are queryable, could we pass down C?

IMO this would be preferable, but this currently leads to the stack-too-deep problem. With Via-IR compilation option turned ON, this would be solved, but currently leads to tests strangely failing.

[GCdePaula]

Even with structs? Which tests are breaking?

[guidanoli]

Even with structs? Which tests are breaking?

Even on main. It breaks testRootWinner with message tournament check-in elapsed.
You can check it for yourself by applying this diff on main:

diff --git a/prt/contracts/foundry.toml b/prt/contracts/foundry.toml
index a7cdd81..1e25967 100644
--- a/prt/contracts/foundry.toml
+++ b/prt/contracts/foundry.toml
@@ -2,6 +2,7 @@
 src = "src"
 out = "out"
 libs = ["lib"]
+via_ir = true
 
 allow_paths = ['../../machine/step/']
 remappings = [

[GCdePaula]

I think this is the issue:

foundry-rs/foundry#7911

We're using vim.warp and block.timestamp. I think we have to update the tests to be correct with via-ir.

[guidanoli]

I think this is the issue

Thanks for the research! I've opened PR #89 to address this.
If it gets merged, I think we can go with the second approach you suggested.

[guidanoli]

Resolved conflicts, but will try to adopt the second approach suggested by @GCdePaula.

[GCdePaula]

Left a few comments! This is looking great.

[GCdePaula]

Hmmm, they should be part of meta-step. This is a "harder" kind of constant, which is really part of the code. It doesn't make a lot of sense to be parametrized.

I think for now we can hardcode them on the leaf tournament. In the future, when we extract the step to the factory, we should put them there. At some point we could make configuring it easier, but I don't think these are values that we need to change a lot.

New releases of the machine could change these parameters. I don't think making them configurable helps our tests .

[GCdePaula]

As a personal style preference, think it's more readable as structs. Something like:

struct TimeConstants {
    Time.Duration matchEffort,
    Time.Duration maxAllowance
}

struct CommitmentStructure {
    uint64 log2step,
    uint64 height
}

struct DisputeParameters {
  TimeConstants timeConstants,
  CommitmentStructure[] commitmentsGranularity
}

[GCdePaula]

If you like the idea of DisputeParameters, I believe that struct would supersede this one. So each level holds its own level plus the DisputeParameters, and can know everything it needs:

• height and log2step become an indexing operation;
• levels queries the size of the array;
• etc.

[guidanoli]

Sorry, I don't understand your comment.

[GCdePaula]

Rereading it is a bit confusing 😅

In a previous comment, I suggested (as a style preference) this approach:

struct TimeConstants {
    Time.Duration matchEffort,
    Time.Duration maxAllowance
}

struct CommitmentStructure {
    uint64 log2step,
    uint64 height
}

struct DisputeParameters {
  TimeConstants timeConstants,
  CommitmentStructure[] commitmentsGranularity
}

The idea is that this single DisputeParameters would be passed down the tournaments, instead of several separate parameters. It's the same thing, but condensed in a single struct. In this proposal, each tournament would hold its own level and the DisputeParameters.

If you like this idea, then the proposed DisputeParameters plus level would supersede the above TournamentParameters, since the former contains the latter in the following way:

tournamentParameters.levels == disputeParameters.commitmentsGranularity.length
tournamentParameters.log2step == disputeParameters.commitmentsGranularity[level].log2step
tournamentParameters.height == disputeParameters.commitmentsGranularity[level].height
tournamentParameters.matchEffort == disputeParameters.timeConstants.matchEffort
tournamentParameters.maxAllowance == disputeParameters.timeConstants.maxAllowance

Extending this idea further, there are other places (i.e. Match) where we store the pair (log2step * height). We could instead use CommitmentStructure. That is, when instantiating a match, we could pass down disputeParameters.commitmentsGranularity[level], instead of "destructuring" it into two integers.

This is also true for the Clock, where we could pass disputeParameters.timeConstants, instead matchEffort and maxAllowance separately.

Let me reiterate, this is a style preference. I think it's ok currently.

[guidanoli]

If we were to pass down this struct to the tournament factory, we would have to read this whole array of log2step-height tuples from storage (because Solidity disallows immutable dynamic arrays), which is costly. In reality, we only need to pass down a single log2step-height tuple.

[GCdePaula]

Gotcha! Makes total sense.

[GCdePaula]

Yeah, also other annoying changes. We'd have to reverse the order in the array of height+log2step, and even worse all the cascading changes offchain.

Let's consider this again later.

[GCdePaula]

I'm not super familiar with Solidity packing. I'm very confused about all this. Here are a bunch of questions.

Does all of this matter if it's immutable? We're using dynamic arrays. Is there really packing of dynamic arrays in storage? Can we actually make dynamic arrays immutable?

I believe static arrays and structs in memory are padded to 32 bytes, but both are packed when in storage. Like, I don't know how much space (uint64, uint64)[] would take in storage when compared with (uint64[], uint64[]). Could the second be worse because we'd be storing the dynamic array's size twice?

My conclusions are:

• I don't think it makes a difference in memory since it's not packed.
• Even if it did, I'm not worried about memory since the size is 3.
• In storage, since the array is dynamic, is it really tightly packed in either alternative?
• It could even be worse to have two dynamic arrays because we'd be storing size twice.

[GCdePaula]

LGTM!

[guidanoli]

Sorry, forgot a constant there. 😅