fix: make block number in txe make sense (#11807)

Right now `env.block_number()` returns the block number that is
currently being built, as per an arbitrary choice when creating the TXe.

But this has a strange side effect of the current block (from the
header) and `env.block_number()` not matching up. The current header has
`env.block_number() - 1` because the current header reflects the state
of the last committed block.

Before this mattered less because we couldn't do historical proofs, and
because we had less of a notion of "correctness" in blocks but now due
to the changes this makes sense to change.

---------

Co-authored-by: benesjan <janbenes1234@gmail.com>

Author: sklppy88

docs/docs/migration_notes.md

@@ -8,6 +8,15 @@ Aztec is in full-speed development. Literally every version breaks compatibility
 
 ## TBD
 
+### [aztec-nr] `TestEnvironment::block_number()` refactored
+
+The `block_number` function from `TestEnvironment` has been expanded upon with two extra functions, the first being `pending_block_number`, and the second being `committed_block_number`. `pending_block_number` now returns what `block_number` does. In other words, it returns the block number of the block we are currently building. `committed_block_number` returns the block number of the last committed block, i.e. the block number that gets used to execute the private part of transactions when your PXE is successfully synced to the tip of the chain.
+
+```diff
++    `TestEnvironment::pending_block_number()`
++    `TestEnvironment::committed_block_number()`
+```
+
 ### [aztec-nr] `compute_nullifier_without_context` renamed
 
 The `compute_nullifier_without_context` function from `NoteHash` (ex `NoteInterface`) is now called `compute_nullifier_unconstrained`, and instead of taking storage slot, contract address and nonce it takes a note hash for nullification (same as `compute_note_hash`). This makes writing this

noir-projects/aztec-nr/aztec/src/state_vars/shared_mutable/test.nr

@@ -50,11 +50,18 @@ unconstrained fn test_get_scheduled_value_in_public() {
     let mut env = setup();
     let state_var = in_public(env);
 
+    // Schedule a value change that will activate at `public_context.block_number() + current_delay`.
+    // Since we haven't modified the delay, it remains at the initial value of TEST_INITIAL_DELAY.
     state_var.schedule_value_change(new_value);
 
     let (scheduled, block_of_change) = state_var.get_scheduled_value();
     assert_eq(scheduled, new_value);
-    assert_eq(block_of_change, env.block_number() + TEST_INITIAL_DELAY);
+
+    // The block of change should equal the pending block number plus TEST_INITIAL_DELAY because:
+    // 1. The value change is scheduled using the block number from the public context
+    // 2. The public context's block number corresponds to the pending block
+    // 3. The current delay is TEST_INITIAL_DELAY since it hasn't been modified
+    assert_eq(block_of_change, env.pending_block_number() + TEST_INITIAL_DELAY);
 }
 
 #[test]
@@ -120,7 +127,7 @@ unconstrained fn test_get_scheduled_delay_in_public() {
     let (scheduled, block_of_change) = state_var.get_scheduled_delay();
     assert_eq(scheduled, new_delay);
     // The new delay is smaller, therefore we need to wait for the difference between current and new
-    assert_eq(block_of_change, env.block_number() + TEST_INITIAL_DELAY - new_delay);
+    assert_eq(block_of_change, env.pending_block_number() + TEST_INITIAL_DELAY - new_delay);
 }
 
 #[test]
@@ -172,7 +179,7 @@ unconstrained fn test_get_current_delay_in_public_after_scheduled_change() {
 unconstrained fn test_get_current_value_in_private_initial() {
     let mut env = setup();
 
-    let historical_block_number = env.block_number();
+    let historical_block_number = env.pending_block_number();
     let state_var = in_private(&mut env, historical_block_number);
 
     assert_eq(state_var.get_current_value(), zeroed());
@@ -191,7 +198,7 @@ unconstrained fn test_get_current_value_in_private_before_change() {
 
     let (_, block_of_change) = public_state_var.get_scheduled_value();
 
-    let schedule_block_number = env.block_number();
+    let schedule_block_number = env.pending_block_number();
 
     let private_state_var = in_private(&mut env, schedule_block_number);
     assert_eq(private_state_var.get_current_value(), MockStruct::empty());

noir-projects/aztec-nr/aztec/src/test/helpers/test_environment.nr

@@ -22,27 +22,60 @@ impl TestEnvironment {
         Self {}
     }
 
-    pub unconstrained fn block_number(_self: Self) -> u32 {
+    /// Returns the block number of the block currently being built. Since sequencer is executing the public part
+    /// of transactions when building blocks, this block number is also returned by public_context.block_number().
+    pub unconstrained fn pending_block_number(_self: Self) -> u32 {
         get_block_number()
     }
 
+    /// This does what the above does but should be considered deprecated as the naming is subpar.
+    pub unconstrained fn block_number(self: Self) -> u32 {
+        self.pending_block_number()
+    }
+
+    /// Returns the block number of the last committed block - this is the block number that gets used in production
+    /// to execute the private part of transactions when your PXE managed to successfully sync to the tip of the chain.
+    pub unconstrained fn committed_block_number(self: Self) -> u32 {
+        self.pending_block_number() - 1
+    }
+
+    /// With TXe tests, every test is run in a mock "contract". This facilitates the ability to write to and read from storage,
+    /// emit and retrieve nullifiers (because they are hashed with a contract address), and formulate notes in a canonical way.
+    /// The contract_address also represents the "msg_sender" when we call a contract with a private or public context; and when
+    /// we call top-level unconstrained functions, we must set this contract address to the contract being called.
+    /// The contract address can be manipulated to do the above at any particular address, and not simply the one provided at
+    /// the instantiation of the test.
+    /// Returns the currently set contract address.
     pub unconstrained fn contract_address(_self: Self) -> AztecAddress {
         get_contract_address()
     }
 
+    /// Modifies the currently set contract address. As per above, it sets the "msg_sender" address on our subsequent calls.
+    /// This is useful when we have multiple "accounts" that need to interact with an arbitrary contract. This also allows
+    /// us to change the "contract" we emit side effects from, and is required when we want to run a top-level unconstrained function on another contract.
     pub unconstrained fn impersonate(_self: Self, address: AztecAddress) {
         cheatcodes::set_contract_address(address)
     }
 
+    /// Advances the internal TXe state to specified block number.
+    /// Note that this block number describes the block being built. i.e. If you advance the block to 5 from 1,
+    /// the pending block number will be 5, and your last committed block number will be 4.
     pub unconstrained fn advance_block_to(&mut self, block_number: u32) {
-        let difference = block_number - get_block_number();
+        let pending_block_number = self.pending_block_number();
+        assert(pending_block_number <= block_number, "Cannot advance block to a previous block.");
+
+        let difference = block_number - pending_block_number;
         self.advance_block_by(difference);
     }
 
+    /// Advances the internal TXe state by the amount of blocks specified. The TXe produces a valid block for every
+    /// block advanced, and we are able to fetch historical data from warped over blocks.
     pub unconstrained fn advance_block_by(_self: &mut Self, blocks: u32) {
         cheatcodes::advance_blocks_by(blocks);
     }
 
+    /// Instantiates a public context. The block number returned from public_context.block_number() will be the
+    /// block number of the block currently being built (same as the one returned by self.pending_block_number()).
     pub unconstrained fn public(_self: Self) -> PublicContext {
         PublicContext::new(|| 0)
     }
@@ -53,15 +86,18 @@ impl TestEnvironment {
         context
     }
 
+    /// Instantiates a private context at the latest committed block number - this is the block number that gets used
+    /// in production to build transactions when your PXE managed to successfully sync to the tip of the chain.
     pub unconstrained fn private(&mut self) -> PrivateContext {
-        self.private_at(get_block_number() - 1)
+        self.private_at(self.committed_block_number())
     }
 
     // unconstrained is a key word, so we mis-spell purposefully here, like we do with contrakt
     pub unconstrained fn unkonstrained(_self: Self) -> UnconstrainedContext {
         UnconstrainedContext::new()
     }
 
+    /// Instantiates a private context at a specific historical block number.
     pub unconstrained fn private_at(&mut self, historical_block_number: u32) -> PrivateContext {
         if historical_block_number >= get_block_number() {
             self.advance_block_to(historical_block_number + 1);

noir-projects/noir-contracts/contracts/router_contract/src/test.nr

@@ -13,8 +13,8 @@ unconstrained fn test_check_block_number() {
     env.advance_block_by(8);
 
     // First we sanity-check that current block number is as expected
-    let current_block_number = env.block_number();
-    assert(current_block_number == 10, "Expected block number to be 10");
+    let current_block_number = env.pending_block_number();
+    assert(current_block_number == 10, "Expected pending block number to be 10");
 
     // We test just one success case and 1 failure case in this test as the rest is tested in the comparator unit tests
     router.check_block_number(Comparator.LT, 11).call(&mut env.private());
@@ -31,7 +31,7 @@ unconstrained fn test_fail_check_block_number() {
     env.advance_block_by(8);
 
     // First we sanity-check that current block number is as expected
-    let current_block_number = env.block_number();
+    let current_block_number = env.pending_block_number();
     assert(current_block_number == 10, "Expected block number to be 10");
 
     router.check_block_number(Comparator.LT, 5).call(&mut env.private());