Skip to content
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.

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

feat(gno.land): support setting custom height/timestamp for genesis txs #2751

Open
wants to merge 55 commits into
base: master
Choose a base branch
from

Conversation

thehowl
Copy link
Member

@thehowl thehowl commented Sep 2, 2024

this is a small PR to allow support for setting TxContexts in the genesis transaction files. Request from #2744:

make block height and timestamp consistent on portal loop (and gnodev? 🙏)

This PR does preparatory work to set that up. With this set up, which has no effect on those who don't use the specific feature (setting tx_contexts), genesis transactions can be executed using custom timestamps and block heights.

To achieve the objective, additional work needs to be carried out on tx-archive, tx-exports and misc/loop, to:

  • tx-archive: record the block time and height of each transaction (IIRC height is already done, block time is missing)
  • tx-exports: have this data in the exports (just to be able to recover the backups, if necessary). Also integrate the portal loop, and integrate it well: create a different directory for each restart, and store the genesis file too, so we have backups for everything even if the PL CI fails.
  • misc/loop: use the data exported by tx-exports to compile the TxContexts in the genesis.json file created at each iteration of the loop.

Note: with this approach, portal loop restarts will still start with genesis height = 0, then start the chain with height = 1. The idea here is that BlockHeights for genesis transactions can also be set to negative values. This means that relative block heights are valid, while their absolute value might change. (Ie. blockHeight1 - blockHeight2, from two different, possibly distant, invocations of std.GetHeight(), will yield the same result; but the specific value of std.GetHeight() changes on portal loop restarts).

Implementation

This code uses issue-2283 (#2319) as a base, to take advantage of the BeginTxHook introduced in that PR. The InitChainerConfig has a field, beginTxHook, that is set each time a genesis transaction is processed. This allows us to inject into the context the ExecContextCustom. Following that, the unified VM Keeper newExecContext will check the sdk.Context to see if there's a ExecContextCustom (only at startup).

Am I particularly proud of this? No, if you can think of a better approach which can have a very simple implementation (note: we need this feature ideally within the next week) and allows us to change the height and timestamp I'm all ears.

One I can think of is to change the context's BlockHeader, putting the values we want for Height/Timestamp; thus changing them at the "tm2" level, rather than at the GnoVM level. This should be harmless; but there's no way to predict and detect misuse (ie. using ctx.BlockHeader().Height(), asserting that this value is provided by the chain and never by the genesis/end-user somehow, which would be the case here). So I'm more tempted to do this.

I attempted to place the values timestamp and height by trying to create a struct that embeds std.Tx and extends it with the two new fields (so as not to touch the normal std.Tx) but unfortunately am̸̭̈ino JSON does not support embedding a struct into another like encoding/json does, so they're just included in the resulting JSON as if they were a normal field. So yes, that road's Too Complex; a different array will have to do the trick 🥲

@thehowl thehowl self-assigned this Sep 2, 2024
@github-actions github-actions bot added 📦 🤖 gnovm Issues or PRs gnovm related 📦 ⛰️ gno.land Issues or PRs gno.land package related labels Sep 2, 2024
Copy link

codecov bot commented Sep 2, 2024

Codecov Report

Attention: Patch coverage is 82.97872% with 8 lines in your changes missing coverage. Please review.

Project coverage is 60.86%. Comparing base (aa4bc99) to head (ed68afa).

Files with missing lines Patch % Lines
gno.land/pkg/sdk/vm/keeper.go 72.41% 7 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2751      +/-   ##
==========================================
+ Coverage   60.83%   60.86%   +0.03%     
==========================================
  Files         563      563              
  Lines       75169    75121      -48     
==========================================
- Hits        45730    45724       -6     
+ Misses      26072    26034      -38     
+ Partials     3367     3363       -4     
Flag Coverage Δ
contribs/gnofaucet 14.46% <ø> (ø)
gnovm 65.59% <ø> (-0.01%) ⬇️
misc/genstd 80.54% <ø> (ø)
misc/logos 19.88% <ø> (-0.36%) ⬇️
tm2 62.11% <ø> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ajnavarro
Copy link
Contributor

The idea here is that BlockHeights for genesis transactions can also be set to negative values

I think we have to be careful with that, because we are representing block heights as unsigned ints everywhere.

@moul
Copy link
Member

moul commented Sep 3, 2024

For the context, here is the previous discussion with the relevant background information: #1511

@moul
Copy link
Member

moul commented Sep 3, 2024

To achieve the objective, additional work needs to be carried out on tx-archive, tx-exports and misc/loop, to:

Ideally, I suggest we sunset misc/loop in favor of a new gnodev staging subcommand. We should focus on making gnodev support both standard developers' use of the more reliable history replay (and sharing) feature, while also enabling the transition of the portal loop to gnodev to simplify our codebase.

Note: with this approach, portal loop restarts will still start with genesis height = 0, then start the chain with height = 1. The idea here is that BlockHeights for genesis transactions can also be set to negative values. This means that relative block heights are valid, while their absolute value might change. (Ie. blockHeight1 - blockHeight2, from two different, possibly distant, invocations of std.GetHeight(), will yield the same result; but the specific value of std.GetHeight() changes on portal loop restarts).

This solution could work, but it sounds rather hacky and may not allow for indexers to properly index the data. Instead, let's try to preserve the history if possible, and use this alternative approach only as a fallback.

Note that @gfanton mentioned that replying with block history works, but it is slow. Perhaps we could introduce a custom flag to enable a "fast and inconsistent" mode for local development, or a "slow but more consistent" mode for environments that require preserving the correct height.

Another consideration is for users who do rely on the height with strict accuracy, such as those using it for random seed initialization or certain DAO patterns. For example, @ilgooz depends on the height for voting, and being negative may not be a problem if we maintain the same "gap." However, it would be preferable to try to preserve the correct height if possible.

if you can think of a better approach which can have a very simple implementation

I need to devote more time to considering this matter. We can discuss it during our next retreat if it hasn't been addressed already.

@thehowl
Copy link
Member Author

thehowl commented Sep 3, 2024

This solution could work, but it sounds rather hacky and may not allow for indexers to properly index the data. Instead, let's try to preserve the history if possible, and use this alternative approach only as a fallback.

okay, I'll look to see if there is a good way we can replay blocks in order, and keep heights consistent.

@moul
Copy link
Member

moul commented Sep 3, 2024

Ideally, indexers like gnoscan could expect a reliable block height while having a special flag like "--ignore-invalid-blocks" to avoid strictly requiring all blocks and correct hashes when processing a reply.

The devnet is a special environment, not a real blockchain, with a single node, so peer-to-peer synchronization is not a priority. For contract writers and tools, a consistent execution context, such as block height and timestamp, would be more crucial.

@moul
Copy link
Member

moul commented Sep 3, 2024

I was considering adding a feature that could facilitate analysis from tools, if it's cost-effective to implement. Otherwise, let's keep it minimal.

If feasible, we could include metadata in the exports specifying whether the transaction was successful or not.

// BlockHeight as the one used by the previous iteration of the chain, as
// from the realm's point of view it will eventually be as if a height
// occurred twice at different times: instead, use negative heights.
// (Last block should be -1, as 0 is the genesis block).
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't very clear to me. Is this saying that transactions will be executed by order of height? So, for example, -5, -4, -3, -2, -1, 0 -- genesis for unspecified transactions?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not necessarily. What I'm trying to communicate is that the keeper/gno.land does not actually "control" the Height; but to avoid collisions with the blocks that are created after genesis, the "space" that should be used for heights should be for negative ones.

But really, this is something that the users of this feature would need to set up with the corresponding values in genesis.json. ie. to guarantee ascending block heights, just order the transactions in a way so that they're ascending.

// occurred twice at different times: instead, use negative heights.
// (Last block should be -1, as 0 is the genesis block).
Height int64 `json:"height"`
Timestamp int64 `json:"timestamp"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it required to have both height and timestamp? Could we just have height and derive timestamps for each height? There could be some wonky behavior here if the tx at height -4 has a timestamp after the tx at height -1, or swap height for timestamp if we choose it replay transactions by timestamp rather than height.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ideally not.

Timestamp != genesisTime + blockHeight * blockTime, because there may be multiple consensus rounds; so it has to be incremented by failedRounds * blockTime at least.

So I'd keep them separate.

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand that is not how timestamp is computed, but what do failed rounds have to do with executing transactions with negative heights? Won't they be executed before genesis? Are there consensus rounds for this? When I asked about deriving timestamps, I was referring to deriving timestamps for transactions with negative heights. I'm just trying to understand the negative heights and how or if there is any relationship between negative height and timestamp and if a transaction with an earlier timestamp can be executed before a transaction with a later timestamp.

Copy link
Member Author

@thehowl thehowl Sep 12, 2024

Choose a reason for hiding this comment

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

When I asked about deriving timestamps, I was referring to deriving timestamps for transactions with negative heights. I'm just trying to understand the negative heights and how or if there is any relationship between negative height and timestamp and if a transaction with an earlier timestamp can be executed before a transaction with a later timestamp.

So, we likely won't be going down the road of negative block heights (the current state of this PR), but I'll still try to answer your question.

This PR per se does not define when a transaction with negative height should happen. It could be at any point in time; the only consequence is, of course, that some realms will expect ascending order for both timestamps and block heights, and have code work under this assumption.

For the use case I envisioned of this feature, though, I wanted to have the result of time.Now() essentially be always the same on the portal loop. So that your transaction is first run on the portal loop at height=10 time=2024-09-12T18:30:00Z; then when the portal loop restarts and all the transactions are dumped into the genesis file, the transaction is run again in the InitChainer with height=-X time=2024-09-12T18:30:00Z; although it's not the "real" block time and height, but rather an emulated one, injected through the TxContexts field in genesis.json

Base automatically changed from dev/morgan/issue-2283 to master September 5, 2024 10:17
@thehowl thehowl requested review from piux2 and a team as code owners September 5, 2024 10:17
@jeronimoalbi
Copy link
Member

Another consideration is for users who do rely on the height with strict accuracy, such as those using it for random seed initialization or certain DAO patterns. For example, @ilgooz depends on the height for voting, and being negative may not be a problem if we maintain the same "gap." However, it would be preferable to try to preserve the correct height if possible.

Just to clarify, we are using timestamps, in case it makes sense to split the issue in two, initially supporting timestamps and eventually implement support for heights.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 ⛰️ gno.land Issues or PRs gno.land package related 📦 🤖 gnovm Issues or PRs gnovm related
Projects
Status: No status
Status: In Review
Development

Successfully merging this pull request may close these issues.

5 participants