Skip to content

consolidations, withdrawals: return type byte at start of output #25

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

consolidations, withdrawals: return type byte at start of output #25

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7fb6a6e
consolidations: whitespace cleanup
fjl Sep 30, 2024
90ac147
withdrawals: return request type byte at start of output
fjl Sep 30, 2024
7371e59
consolidations: return request type byte at start of output
fjl Sep 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 21 additions & 9 deletions src/consolidations/main.eas
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,13 +29,15 @@
#define INPUT_SIZE 96 ;; the size of (source ++ target)
#define RECORD_SIZE 116 ;; the size of (address ++ source ++ target)

#define REQUEST_TYPE 0x02

;; -----------------------------------------------------------------------------
;; PROGRAM START ---------------------------------------------------------------
;; -----------------------------------------------------------------------------

.start:
;; Protect the system subroutine by checking if the caller is the system
;; address.
;; address.
caller ;; [caller]
push20 SYSTEM_ADDR ;; [sysaddr, caller]
eq ;; [sysaddr == caller]
Expand Down Expand Up @@ -184,7 +186,7 @@ check_input:
;; with each record being exactly RECORD_SIZE bytes.
;;
;; Consolidation request record:
;;
;;
;; +------+--------+--------+
;; | addr | source | target |
;; +------+--------+--------+
Expand Down Expand Up @@ -220,7 +222,13 @@ read_requests:
push MAX_PER_BLOCK ;; [count, head_idx, tail_idx]

begin_loop:
push0 ;; [i, count, head_idx, tail_idx]
;; Store request type prefix into the output buffer.
push1 REQUEST_TYPE ;; [type, count, head_idx, tail_idx]
push 0 ;; [offset, type, count, head_idx, tail_idx]
mstore8 ;; [count, head_idx, tail_idx]

;; Push initial loop index.
push 0 ;; [i, count, head_idx, tail_idx]

accum_loop:
;; This loop will read each request and byte bang it into a RECORD_SIZE byte chunk.
Expand All @@ -234,7 +242,9 @@ accum_loop:
;; Precompute record_offset = i*RECORD_SIZE.
dup1 ;; [i, i, count, head_idx, tail_idx]
push RECORD_SIZE ;; [size, i, i, count, head_idx, tail_idx]
mul ;; [record_offset, i, count, head_idx, tail_idx]
mul ;; [offset, i, count, head_idx, tail_idx]
push 1 ;; [1, offset, i, count, head_idx, tail_idx]
add ;; [record_offset, i, count, head_idx, tail_idx]
Copy link
Member

@lightclient lightclient Sep 30, 2024

Choose a reason for hiding this comment

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

I wonder if it is better to add a loop-level item record_offset and just add RECORD_SIZE during each iteration. Feels weird to +1 every calculation.

Copy link
Collaborator Author

@fjl fjl Sep 30, 2024

Choose a reason for hiding this comment

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

Yeah, so that would work as well. Let me know if you prefer this.

Copy link
Collaborator Author

@fjl fjl Sep 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I even thought about turning the whole thing around to keep the offset permanently near the stack top and increment after every store. But then we'd need to find a clean way to hoist up the items from below.

Copy link
Member

@lightclient lightclient Oct 1, 2024

Choose a reason for hiding this comment

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

Eh looking into this more, this is pretty straightforward and understandable implementation. I think we can merge as-is and decide later if it makes sense to optimize it.

Copy link
Collaborator Author

@fjl fjl Oct 1, 2024

Choose a reason for hiding this comment

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

It's less about optimizing and more about correctness. The advantage of a rolling offset is that we only need to add the length of the current item after writing the item. It removes the hard-coded offsets. But I can look into this as a refactoring after this PR is in.


;; Determine the storage slot of the address for this iteration. This value is
;; also the base for the other storage slots containing the source and the target
Expand Down Expand Up @@ -280,7 +290,7 @@ accum_loop:
swap3 ;; [addr, src[32:48] ++ tgt[0:16], source[0:32], target[16:32], record_offset, i, ..]
push 12*8 ;; [96, addr, src[32:48] ++ tgt[0:16], source[0:32], target[16:32], record_offset, i, ..]
shl ;; [addr<<96, src[32:48] ++ tgt[0:16], source[0:32], target[16:32], record_offset, i, ..]

;; Store addr at offset = i*RECORD_SIZE.
dup5 ;; [record_offset, addr<<96, src[32:48] ++ tgt[0:16], source[0:32], target[16:32], record_offset, i, ..]
mstore ;; [src[32:48] ++ tgt[0:16], source[0:32], target[16:32], record_offset, i, ..]
Expand Down Expand Up @@ -346,7 +356,7 @@ update_excess:
;; Update the new excess withdrawal requests.
push SLOT_EXCESS ;; [excess_slot, count]
sload ;; [excess, count]

;; Check if excess needs to be reset to 0 for first iteration after
;; activation.
dup1 ;; [excess, excess, count]
Expand All @@ -372,11 +382,11 @@ skip_reset:
add ;; [count+excess, target, count, excess, count]
gt ;; [count+excess > target, count, excess, count]
jumpi @compute_excess ;; [count, excess, count]

;; Zero out excess.
pop ;; [excess, count]
pop ;; [count]
push0
push0
jump @store_excess

compute_excess:
Expand All @@ -396,7 +406,9 @@ store_excess:

;; Return the requests.
push RECORD_SIZE ;; [record_size, count]
mul ;; [size]
mul ;; [size, count]
push 1 ;; [1, size, count]
add ;; [size+1, count]
push0 ;; [0, size]
return ;; []

Expand Down
20 changes: 16 additions & 4 deletions src/withdrawals/main.eas
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@
#define INPUT_SIZE 56 ;; the size of (pubkey ++ amount)
#define RECORD_SIZE 76 ;; the size of (address ++ pubkey ++ amount)

#define REQUEST_TYPE 0x01

;; -----------------------------------------------------------------------------
;; PROGRAM START ---------------------------------------------------------------
;; -----------------------------------------------------------------------------
Expand Down Expand Up @@ -220,7 +222,13 @@ read_requests:
push MAX_PER_BLOCK ;; [count, head_idx, tail_idx]

begin_loop:
push0 ;; [i, count, head_idx, tail_idx]
;; Store request type prefix into the output buffer.
push1 REQUEST_TYPE ;; [type, count, head_idx, tail_idx]
push 0 ;; [offset, type, count, head_idx, tail_idx]
mstore8 ;; [count, head_idx, tail_idx]

;; Push initial loop index.
push 0 ;; [i, count, head_idx, tail_idx]

accum_loop:
;; This loop will read each request and byte bang it into a 76 byte chunk.
Expand All @@ -231,10 +239,12 @@ accum_loop:
eq ;; [i == count, i, count, head_idx, tail_idx]
jumpi @update_head ;; [i, count, head_idx, tail_idx]

;; Precompute record_offset = i*RECORD_SIZE.
;; Precompute record_offset = i*RECORD_SIZE + 1.
dup1 ;; [i, i, count, head_idx, tail_idx]
push RECORD_SIZE ;; [size, i, i, count, head_idx, tail_idx]
mul ;; [record_offset, i, count, head_idx, tail_idx]
mul ;; [offset, i, count, head_idx, tail_idx]
push 1 ;; [1, offset, i, count, head_idx, tail_idx]
add ;; [record_offset, i, count, head_idx, tail_idx]

;; Determine the storage slot of the address for this iteration. This value is
;; also the base for the other two storage slots containing the public key and
Expand Down Expand Up @@ -414,7 +424,9 @@ store_excess:

;; Return the withdrawal requests.
push RECORD_SIZE ;; [record_size, count]
mul ;; [size]
mul ;; [size, count]
push 1 ;; [1, size, count]
add ;; [size+1, count]
push0 ;; [0, size]
return ;; []

Expand Down
13 changes: 6 additions & 7 deletions test/Consolidation.t.sol.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,11 +50,10 @@ contract ConsolidationTest is Test {
assertExcess(0);

bytes memory req = getRequests();
assertEq(req.length, 116);
assertEq(toFixed(req, 20, 52), toFixed(data, 0, 32));
assertEq(toFixed(req, 52, 84), toFixed(data, 32, 64));
assertEq(toFixed(req, 84, 116), toFixed(data, 64, 96));
assertStorage(count_slot, 0, "unexpected request count");
assertEq(req.length, 117);
assertEq(uint8(req[0]), 0x02);
assertEq(slice(req, 21, 117), data, "wrong output request data");
assertStorage(count_slot, 0, "unexpected request count in storage");
assertExcess(0);
}

Expand Down Expand Up @@ -196,9 +195,9 @@ contract ConsolidationTest is Test {
// uint8(index), repeating.
function checkConsolidations(uint256 startIndex, uint256 count) internal returns (uint256) {
bytes memory requests = getRequests();
assertEq(requests.length, count*116);
assertEq(requests.length, count*116 + 1);
for (uint256 i = 0; i < count; i++) {
uint256 offset = i*116;
uint256 offset = i*116 + 1;
assertEq(toFixed(requests, offset, offset+20) >> 96, uint256(startIndex+i), "unexpected request address returned");
assertEq(toFixed(requests, offset+20, offset+52), toFixed(makeConsolidation(startIndex+i), 0, 32), "unexpected source[0:32] returned");
assertEq(toFixed(requests, offset+52, offset+84), toFixed(makeConsolidation(startIndex+i), 32, 64), "unexpected source[32:48] ++ target[0:16] returned");
Expand Down
11 changes: 11 additions & 0 deletions test/Test.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,17 @@ function toFixed(bytes memory data, uint256 start, uint256 end) pure returns (ui
return uint256(bytes32(out));
}

// slice copys data from memory. If the length is less than 32,
// the output is right-padded with zeros.
function slice(bytes memory data, uint start, uint end) pure returns (bytes memory) {
require(end > start, "invalid range (end > start)");
bytes memory out = new bytes(end-start);
for (uint i = start; i < end; i++) {
out[i-start] = data[i];
}
return out;
}

// computeFee calls the fake exponentiation contract with the specified
// parameters to determine the correctt fee value.
function computeFee(uint256 excess) returns (uint256) {
Expand Down
16 changes: 9 additions & 7 deletions test/Withdrawal.t.sol.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,11 +52,12 @@ contract WithdrawalsTest is Test {
assertExcess(0);

bytes memory req = getRequests();
assertEq(req.length, 76);
assertEq(bytes20(req), bytes20(address(this))); // check addr
assertEq(toFixed(req, 20, 52), toFixed(exp_req, 0, 32)); // check pk1
assertEq(toFixed(req, 52, 68), toFixed(exp_req, 32, 48)); // check pk2
assertEq(toFixed(req, 68, 76), toFixed(exp_req, 48, 56)); // check amt
assertEq(req.length, 77);
assertEq(uint8(req[0]), 0x01, "wrong request type prefix");
assertEq(bytes20(slice(req, 1, 21)), bytes20(address(this)), "wrong address");
assertEq(toFixed(req, 21, 53), toFixed(exp_req, 0, 32), "wrong pk1");
assertEq(toFixed(req, 53, 69), toFixed(exp_req, 32, 48), "wrong pk2");
assertEq(toFixed(req, 69, 77), toFixed(exp_req, 48, 56), "wrong amount");
assertStorage(count_slot, 0, "unexpected request count");
assertExcess(0);
}
Expand Down Expand Up @@ -197,10 +198,11 @@ contract WithdrawalsTest is Test {
function checkWithdrawals(uint256 startIndex, uint256 count) internal returns (uint256) {
bytes memory amountBuffer = new bytes(8);
bytes memory requests = getRequests();
assertEq(requests.length, count*76);
assertEq(requests.length, count*76 + 1);
assertEq(uint8(requests[0]), 0x01);

for (uint256 i = 0; i < count; i++) {
uint256 offset = i*76;
uint256 offset = i*76 + 1;
uint256 wdIndex = startIndex + i;
bytes memory wd = makeWithdrawal(wdIndex);

Expand Down
Loading