Skip to content

Commit

Permalink
feat: Removing is_dev_net flag (#8275)
Browse files Browse the repository at this point in the history
This PR tries to get rid of the `is_dev_net` flag that we had in the
`constants.nr`. This is to simplify the flows such that is just one flow
used by both the devnet and spartan.

Alters our `createNode` and `setup` such that they will always assume
active validators, since it will be required for proper sequencing.

Changes the `l1-publisher` slightly to reduce the probability that
`validateBlockForSubmission` would fail due to an Ethereum block
arriving between the check and the `propose` call.

Alters the `sequencer::work()` function such that there is a cleaner
split between when we can return early, and when we have to throw an
error and revert to properly rollback the global state.

Alters the `collectAttestations` functions slightly, i) to cover the
case where a validator client is not provided but a committee is needed,
and ii) to ensure that the sequencers own attestation also makes it way
into the attestations collected.

---

# Graveyard
Below this point is the graveyard where old issues very talked about and
insanity ensued.

--- 

Currently running into issues where tests are behaving "strange".
Namely, it seems like we sometimes will have a passing test and
sometimes wont.

This especially is encountered when many tests are run at once, such as
the `e2e_token_contract` tests. A snippet below shares some frustration.
If we are running all the tests, they always fail, but if running only
some, it seems to depend on what is being logged...

```bash
DEBUG="aztec:*,-aztec:avm_simulator:*" LOG_LEVEL="silent" yarn test e2e_token_contract/transfer_private // passes
LOG_LEVEL="silent" yarn test e2e_token_contract/transfer_private // fails
LOG_LEVEL="DEBUG" yarn test e2e_token_contract/transfer_private // fails
```

Somewhat interesting, if using `AZTEC_SLOT_DURATION = 36` many of these
issues seems to go away, e.g., transactions are not dropped anymore etc.
However, this really should not be the case, hence this time influence
is not walltime, as it is using an anvil instance behind the scenes.
Main reason around this is more likely to be that we don't encounter the
case where a `submitProof` have progressed time and moved the slot.

--- 
Looking at logs!

What do I see in the logs
- Transaction TX_A is dropped.
- When looking higher, I can see that TX_A is dropped because of
duplicate nullifiers in the state trees! Something interesting! While
the nullifier tree from sync is size 1088, the one we match against is
1216 and the index of first collisions is OUTSIDE of the tree that you
get from synching :skull:
- Looking JUST above where we drop these transactions i see that we are
encountering an error while perfoming `validateHeader` (the slot have
changed because of `submitProof`)
- Looking slightly above this, we can see what I believe is the
sequencer simulating the base rollups (all good here!)
- Moving slightly up, we can see that the sequencer is processing the
transaction itself.
- Further up, we see the user creating the transaction
- And above that we see the last block, lets call it BLOCK_A

Note from this: There is no block after BLOCK_A where TX_A could have
been included, but when the sequencer is FAILING to publish the new
block, it seems to be keeping the state but dropping the block AND its
transactions. So the setup fails because the user will get the response
from the node that "this is a double-spend, go away".

I tried using `PROVER_NODE_DISABLE_AUTOMATIC_PROVING` to turn of the
proving, but that don't seem to have any effect, and if I try to just
bypass the submitProofs it seems to cause the application to infinite
loop where it just never tries anything ever again.

The tree that the sequencer is checking against that is larger than what
you get from sync seem to only be larger for a "short" time before it
figures out something is messed up, and will "rollback", but the damage
is done and we just dropped potentially a lot of transactions

There exact timing of the failure also "depends", so that is kinda pain.


![image](https://github.com/user-attachments/assets/2fad7185-fb32-4ffd-a825-e9c55263c8e3)



**Update**: 
Issue seemed to be that if we returned early from `work()` in the
sequencer, when not proposing a block. We would not rollback the state,
so if state changes were made, it would WRECK the next block.
  • Loading branch information
LHerskind authored Aug 30, 2024
1 parent 41d418c commit fc1f307
Show file tree
Hide file tree
Showing 19 changed files with 302 additions and 613 deletions.
59 changes: 6 additions & 53 deletions l1-contracts/src/core/Rollup.sol
Original file line number Diff line number Diff line change
Expand Up @@ -67,10 +67,6 @@ contract Rollup is Leonidas, IRollup, ITestRollup {

bytes32 public vkTreeRoot;

// @note This should not exists, but we have it now to ensure we will not be killing the devnet with our
// timeliness requirements.
bool public isDevNet = Constants.IS_DEV_NET == 1;

// @note Assume that all blocks up to this value are automatically proven. Speeds up bootstrapping.
// Testing only. This should be removed eventually.
uint256 private assumeProvenUntilBlockNumber;
Expand Down Expand Up @@ -111,12 +107,10 @@ contract Rollup is Leonidas, IRollup, ITestRollup {
* @notice Prune the pending chain up to the last proven block
*
* @dev Will revert if there is nothing to prune or if the chain is not ready to be pruned
*
* @dev While in devnet, this will be guarded behind an `onlyOwner`
*/
function prune() external override(IRollup) {
if (isDevNet) {
revert Errors.DevNet__NoPruningAllowed();
}

function prune() external override(IRollup) onlyOwner {
if (pendingBlockCount == provenBlockCount) {
revert Errors.Rollup__NothingToPrune();
}
Expand Down Expand Up @@ -155,17 +149,6 @@ contract Rollup is Leonidas, IRollup, ITestRollup {
assumeProvenUntilBlockNumber = blockNumber;
}

/**
* @notice Set the devnet mode
*
* @dev This is only needed for testing, and should be removed
*
* @param _devNet - Whether or not the contract is in devnet mode
*/
function setDevNet(bool _devNet) external override(ITestRollup) onlyOwner {
isDevNet = _devNet;
}

/**
* @notice Set the verifier contract
*
Expand Down Expand Up @@ -412,13 +395,9 @@ contract Rollup is Leonidas, IRollup, ITestRollup {
revert Errors.Rollup__InvalidArchive(tipArchive, _archive);
}

if (isDevNet) {
_devnetSequencerSubmissionChecks(_proposer);
} else {
address proposer = getProposerAt(_ts);
if (proposer != address(0) && proposer != _proposer) {
revert Errors.Leonidas__InvalidProposer(proposer, _proposer);
}
address proposer = getProposerAt(_ts);
if (proposer != address(0) && proposer != _proposer) {
revert Errors.Leonidas__InvalidProposer(proposer, _proposer);
}

return (slot, pendingBlockCount);
Expand Down Expand Up @@ -568,8 +547,6 @@ contract Rollup is Leonidas, IRollup, ITestRollup {
* This might be relaxed for allow consensus set to better handle short-term bursts of L1 congestion
* - The slot MUST be in the current epoch
*
* @dev While in isDevNet, we allow skipping all of the checks as we simply assume only TRUSTED sequencers
*
* @param _slot - The slot of the header to validate
* @param _signatures - The signatures to validate
* @param _digest - The digest that signatures sign over
Expand All @@ -581,19 +558,6 @@ contract Rollup is Leonidas, IRollup, ITestRollup {
uint256 _currentTime,
DataStructures.ExecutionFlags memory _flags
) internal view {
if (isDevNet) {
// @note If we are running in a devnet, we don't want to perform all the consensus
// checks, we instead simply require that either there are NO validators or
// that the proposer is a validator.
//
// This means that we relaxes the condition that the block must land in the
// correct slot and epoch to make it more fluid for the devnet launch
// or for testing.

_devnetSequencerSubmissionChecks(msg.sender);
return;
}

// Ensure that the slot proposed is NOT in the future
uint256 currentSlot = getSlotAt(_currentTime);
if (_slot != currentSlot) {
Expand Down Expand Up @@ -687,15 +651,4 @@ contract Rollup is Leonidas, IRollup, ITestRollup {
revert Errors.Rollup__UnavailableTxs(_header.contentCommitment.txsEffectsHash);
}
}

function _devnetSequencerSubmissionChecks(address _proposer) internal view {
if (getValidatorCount() == 0) {
return;
}

if (!isValidator(_proposer)) {
revert Errors.DevNet__InvalidProposer(getValidatorAt(0), _proposer);
}
return;
}
}
1 change: 0 additions & 1 deletion l1-contracts/src/core/interfaces/IRollup.sol
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import {SignatureLib} from "../sequencer_selection/SignatureLib.sol";
import {DataStructures} from "../libraries/DataStructures.sol";

interface ITestRollup {
function setDevNet(bool _devNet) external;
function setVerifier(address _verifier) external;
function setVkTreeRoot(bytes32 _vkTreeRoot) external;
function setAssumeProvenUntilBlockNumber(uint256 blockNumber) external;
Expand Down
1 change: 0 additions & 1 deletion l1-contracts/src/core/libraries/ConstantsGen.sol
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,6 @@ library Constants {
uint256 internal constant ETHEREUM_SLOT_DURATION = 12;
uint256 internal constant AZTEC_SLOT_DURATION = 12;
uint256 internal constant AZTEC_EPOCH_DURATION = 48;
uint256 internal constant IS_DEV_NET = 1;
uint256 internal constant GENESIS_ARCHIVE_ROOT =
8142738430000951296386584486068033372964809139261822027365426310856631083550;
uint256 internal constant FEE_JUICE_INITIAL_MINT = 20000000000;
Expand Down
11 changes: 0 additions & 11 deletions l1-contracts/test/Rollup.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -113,13 +113,6 @@ contract RollupTest is DecoderBase {
}

function testRevertPrune() public setUpFor("mixed_block_1") {
if (rollup.isDevNet()) {
vm.expectRevert(abi.encodeWithSelector(Errors.DevNet__NoPruningAllowed.selector));
rollup.prune();

return;
}

vm.expectRevert(abi.encodeWithSelector(Errors.Rollup__NothingToPrune.selector));
rollup.prune();

Expand All @@ -136,10 +129,6 @@ contract RollupTest is DecoderBase {
}

function testPrune() public setUpFor("mixed_block_1") {
if (rollup.isDevNet()) {
return;
}

_testBlock("mixed_block_1", false);

assertEq(inbox.inProgress(), 3, "Invalid in progress");
Expand Down
232 changes: 0 additions & 232 deletions l1-contracts/test/sparta/DevNet.t.sol

This file was deleted.

Loading

0 comments on commit fc1f307

Please sign in to comment.