fix: make block number in txe make sense

[sklppy88]

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.

[sklppy88]

• fix: make block number in txe make sense #11807 : 2 dependent PRs (#11711 , #12215 ) 👈 (View in Graphite)
• master

This stack of pull requests is managed by Graphite. Learn more about stacking.

[sklppy88]

Because now env.block_number() to the most recent committed block, the initial delay needs to be added the block that this tx will be committed in (which is env.block_number() + 1.

[benesjan]

Nobody reading the test would have any clue why this is the case. We need better naming here.

[sklppy88]

Good point, have reworked this a bit.

[benesjan]

This function needs to be documented. It's also just weird that we have a env.block_number() function and it returns different thing from get_block_number() oracle.

I think we should rename TestEnvironment::block_number to TestEnvironment::committed_block_number or smt. Or alternatively to historical_block_number since we call it like that here.

Also do you know what block number would be used in private calls and what be used in public calls? Would it be the case that in private we would have output of env.block_number() and in public it would be incremented by 1?

[iAmMichaelConnor]

I agree. My intuition would be that .block_number would return the current block in which the tx is being "mined".

Something like previous_block_number or latest_committed_block_number might work for the prev block number

Presumably this function is only accessible in a Public context? In private, there is no notion of "the current block number" or "the previous block number", because it's computed async from the chain. Therefore, I think these functions should throw if called from a Private context.

[sklppy88]

Thanks for the comments guys, you make some very good points.

Currently in the codebase, the get_block_number oracle only exists in tests and when creating an unconstrained context.

Also, it is not possible to call env.block_number() in contract functions of any type (nor should you want to).

It is not possible to call the oracle get_block_number in contract functions of public type.
We get a panic on compile because it can't resolve the oracle. But it is possible to call this in private and unconstrained. When it's used in unconstrained / private context, it looks at the ViewDataOracle's node for the source of the block number, and it seems to use the last processed block, not the block currently being built, unless I have a misunderstanding about this.

Code snippet below illustrating the above:

   * Gets the number of the latest L2 block processed by the block source implementation.
   * @returns The number of the latest L2 block processed by the block source implementation.
   */

Between our different execution environments, the oracle itself returns something different. In the TXe it uses the block number of the block being built, but in an actual unconstrained / private environment it seemingly uses the last known block from our connected node.

If you are referring to the actual way the block number is fetched for private calls in the TXe, (if you get the context object with env.private()) is get_block_number() - 1. The block for any enqueued public calls is the block number currently being built. These are accessed by env.private().historical_header.global_variables.block_number, and context.get_block_number() respectively.

If you are testing things like state vars through the TXe directly, you will get the same value when using the get_block_number() oracle (which is the block number of the block being built), regardless of what the context is relevant to (private or public), as it will use the internal TXe oracle resolver.

Ultimately though I agree that

1. the naming is very confusing and needs addressing and
2. that we might want to block the oracle from calling it in private (we don't need to worry about env.get_block_number() because this cannot be called from private).

For 1, I have addressed this by splitting it into two functions to disambiguate:

pending_block_number <- block number being built
committed_block_number <- block number of the last committed block

(I don't think historical block number is good as I feel like it is too vague, it is used a bit willy nilly, as evidenced by this.)

For 2, I can open a PR that essentially overrides the getBlockNumber oracle in ClientExecutionContext that throws an error and says this oracle is not available in private.

Wdyt ?

[benesjan]

Nobody reading the test would have any clue why this is the case. We need better naming here.

[benesjan]

Looks great. Thanks.

Just please add migration notes as the function renaming is a breaking change