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

post-state-root: implement block production #9618

Closed
Tracked by #9450
pugachAG opened this issue Sep 29, 2023 · 2 comments
Closed
Tracked by #9450

post-state-root: implement block production #9618

pugachAG opened this issue Sep 29, 2023 · 2 comments
Assignees
Labels
A-chain Area: Chain, client & related Near Core

Comments

@pugachAG
Copy link
Contributor

Extend block production logic to support post-state-root version

@Longarithm
Copy link
Member

Longarithm commented Oct 4, 2023

My summary of necessary changes:

  • prev_validator_proposals field goes to BlockInfo::proposals, used in epoch manager, validated in validate_chunk_headers. It can stay unchanged, but it feels more natural to propagate most recent proposals, available in validator_proposals in post-state-root world. New name should be validator_proposals. Edge case: on first block with post-state-root we should add both prev and current proposals.
  • gas_price - this will not define gas_price in post-state-root world, so I would change it to next_gas_price to reflect reality. And use newly introduced gas_used and balance_burnt, indeed. Edge case: on first block with post-state-root, aggregate both prev_gas_used and gas_used.

Not that necessary changes - we can remove challenges_root and challenges_result, as we don't plan to implement challenges anymore. We could consider the same about block_ordinal and epoch_sync_data_hash.

Full fields description:

BlockHeaderInnerLite

  • height - very widely used. Defines block/chunk producer, finality conditions. In pre-state-root world defines height of chunk inclusion. No need to change.
  • epoch_id - defines current validator set. Can be retrieved using prev block hashes until we reach epoch start, but it’s okay to keep signing it. No need to change.
  • next_epoch_id - similar, not impacted by post-state-root.
  • prev_state_root - can be retrieved by ShardChunkHeaders, but actually needed for light clients as we want them to process as few data as possible. Can be changed to post_state_root later, but bridges may need to be updated. Depends on how field is used.
  • prev_outcome_root - same as above.
  • timestamp - called in BlockHeader::timestamp. Participates in validation conditions: 1) ts are strictly increasing; 2) not increases by more than 2 minutes at once. Exposed to contracts. For post-state-root, we may change timestamp passed to runtime:
    let block_timestamp = block.header().raw_timestamp();
    by passing prev block timestamp instead, to be consistent with old API. But field doesn't change, except maybe adding 1 unit on edge case.
  • next_bp_hash - doesn't change.
  • block_merkle_root - doesn't change.

BlockHeaderInnerRestV5

  • block_body_hash - doesn't change
  • prev_chunk_outgoing_receipts_root - doesn't change. Publicly used ONLY for state sync, see ShardStateSyncResponseHeader. Not sure why it is needed there. Can be superseded by new field outgoing_receipts_root which would make more sense.
  • chunk_headers_root - doesn't change. Maybe it is needed for faster state sync to prove only one chunk header without need to download all chunk headers.
  • chunk_tx_root - doesn't change and has no usecases. Block body has chunk headers anyway.
  • challenges_root - will be useless
  • random_value - needs to change
  • prev_validator_proposals - see above.
  • chunk_mask - main purpose is to init BlockInfo::chunk_mask and compute produced/expected stats. No need to change.
  • gas_price - see above.
  • total_supply - is adjusted by minted_amount on every epoch start. Can be retrieved from epoch start, but I don't mind signing it each block.
  • challenges_result - will be useless
  • last_final_block - used very widely. I think it is very useful, as it gives finality info right away. We don't need to change it as stateless validation has nothing to do with finality.
  • last_ds_final_block - used to recompute last_final_block, stays as is
  • block_ordinal - unused, unchanged
  • prev_height - I see Robin touched it recently. Looks like it is used to determine approval kind, this is fine.
  • epoch_sync_data_hash - unused
  • approvals - will contain chunk endorsements later, but for now has only block approvals which don't change.
  • latest_protocol_version - stays as is.

@Longarithm
Copy link
Member

After a bit of discussions, I think that there are no necessary fields to change.

Explanation

Validator proposals

Let's stop caring about what's the name of the field. Focus on invariant instead, which is:

Validator proposals must be applied at the same time to EpochManager (during add_validator_proposals) and State (during chunk processing)
This is handled by 2-staged processing of proposals:

  1. Stake Action is processed during applying chunk. Its result is saved as "validator proposal" and stored to ApplyChunkResult, which then goes to ChunkExtra.
  2. On applying next block we take these previously saved proposals and:
    2a) apply them to EpochManager
    2b) apply them to State during processing of respective chunk

So nothing changes here. With post-state-root, we can just pass proposals using chunk header itself (new validator_proposals field), not chunk extra. Changing block header contents would be actually a mistake.

Gas price

I assumed that in pre-state-root, for applying chunk, gas price is taken from the block which contains this chunk, as we already know the block.
But it’s not the case. We take gas price from previous block:

let gas_price = prev_block.header().gas_price();

I don't know why. But in both pre and post state roots previous block exists anyway, so we can avoid any changes there.
(@pugachAG: we can still just rename this field to next_gas_price to avoid further confusion)

github-merge-queue bot pushed a commit that referenced this issue Oct 6, 2023
Part of #9618.

In post-state-root, chunk production is moved before block production,
thus we don't have a `Block` during applying chunk anymore. We need to
make sure that all necessary data is still propagated to
`Runtime::apply`.

It should've not have been an error for gas price, as it is taken from
previous block in the general case:
https://github.com/near/nearcore/blob/9c8d0bbec6a71876bb3317db2ade425f0077a184/chain/chain/src/chain.rs#L4120
However, when there is no chunk, there seem to be a very old error - it
is taken from the same block.

Luckily, after `ProtocolFeature::FixApplyChunks` it is not an issue:
when there is no chunk, gas price is not used at all. So I just start
taking it from previous block for consistency, to ensure that in
post-state-root current block can be safely removed.

Full context:
https://near.zulipchat.com/#narrow/stream/295302-general/topic/.E2.9C.94.20gas.20price.20inconsistency/near/394894144

## Testing

As the change must be no-op, existing tests are enough.
nikurt pushed a commit that referenced this issue Oct 9, 2023
Part of #9618.

In post-state-root, chunk production is moved before block production,
thus we don't have a `Block` during applying chunk anymore. We need to
make sure that all necessary data is still propagated to
`Runtime::apply`.

It should've not have been an error for gas price, as it is taken from
previous block in the general case:
https://github.com/near/nearcore/blob/9c8d0bbec6a71876bb3317db2ade425f0077a184/chain/chain/src/chain.rs#L4120
However, when there is no chunk, there seem to be a very old error - it
is taken from the same block.

Luckily, after `ProtocolFeature::FixApplyChunks` it is not an issue:
when there is no chunk, gas price is not used at all. So I just start
taking it from previous block for consistency, to ensure that in
post-state-root current block can be safely removed.

Full context:
https://near.zulipchat.com/#narrow/stream/295302-general/topic/.E2.9C.94.20gas.20price.20inconsistency/near/394894144

## Testing

As the change must be no-op, existing tests are enough.
nikurt pushed a commit that referenced this issue Oct 11, 2023
Part of #9618.

In post-state-root, chunk production is moved before block production,
thus we don't have a `Block` during applying chunk anymore. We need to
make sure that all necessary data is still propagated to
`Runtime::apply`.

It should've not have been an error for gas price, as it is taken from
previous block in the general case:
https://github.com/near/nearcore/blob/9c8d0bbec6a71876bb3317db2ade425f0077a184/chain/chain/src/chain.rs#L4120
However, when there is no chunk, there seem to be a very old error - it
is taken from the same block.

Luckily, after `ProtocolFeature::FixApplyChunks` it is not an issue:
when there is no chunk, gas price is not used at all. So I just start
taking it from previous block for consistency, to ensure that in
post-state-root current block can be safely removed.

Full context:
https://near.zulipchat.com/#narrow/stream/295302-general/topic/.E2.9C.94.20gas.20price.20inconsistency/near/394894144

## Testing

As the change must be no-op, existing tests are enough.
nikurt pushed a commit that referenced this issue Oct 16, 2023
Part of #9618.

In post-state-root, chunk production is moved before block production,
thus we don't have a `Block` during applying chunk anymore. We need to
make sure that all necessary data is still propagated to
`Runtime::apply`.

It should've not have been an error for gas price, as it is taken from
previous block in the general case:
https://github.com/near/nearcore/blob/9c8d0bbec6a71876bb3317db2ade425f0077a184/chain/chain/src/chain.rs#L4120
However, when there is no chunk, there seem to be a very old error - it
is taken from the same block.

Luckily, after `ProtocolFeature::FixApplyChunks` it is not an issue:
when there is no chunk, gas price is not used at all. So I just start
taking it from previous block for consistency, to ensure that in
post-state-root current block can be safely removed.

Full context:
https://near.zulipchat.com/#narrow/stream/295302-general/topic/.E2.9C.94.20gas.20price.20inconsistency/near/394894144

## Testing

As the change must be no-op, existing tests are enough.
nikurt pushed a commit that referenced this issue Oct 19, 2023
Part of #9618.

In post-state-root, chunk production is moved before block production,
thus we don't have a `Block` during applying chunk anymore. We need to
make sure that all necessary data is still propagated to
`Runtime::apply`.

It should've not have been an error for gas price, as it is taken from
previous block in the general case:
https://github.com/near/nearcore/blob/9c8d0bbec6a71876bb3317db2ade425f0077a184/chain/chain/src/chain.rs#L4120
However, when there is no chunk, there seem to be a very old error - it
is taken from the same block.

Luckily, after `ProtocolFeature::FixApplyChunks` it is not an issue:
when there is no chunk, gas price is not used at all. So I just start
taking it from previous block for consistency, to ensure that in
post-state-root current block can be safely removed.

Full context:
https://near.zulipchat.com/#narrow/stream/295302-general/topic/.E2.9C.94.20gas.20price.20inconsistency/near/394894144

## Testing

As the change must be no-op, existing tests are enough.
nikurt pushed a commit that referenced this issue Oct 23, 2023
Part of #9618.

In post-state-root, chunk production is moved before block production,
thus we don't have a `Block` during applying chunk anymore. We need to
make sure that all necessary data is still propagated to
`Runtime::apply`.

It should've not have been an error for gas price, as it is taken from
previous block in the general case:
https://github.com/near/nearcore/blob/9c8d0bbec6a71876bb3317db2ade425f0077a184/chain/chain/src/chain.rs#L4120
However, when there is no chunk, there seem to be a very old error - it
is taken from the same block.

Luckily, after `ProtocolFeature::FixApplyChunks` it is not an issue:
when there is no chunk, gas price is not used at all. So I just start
taking it from previous block for consistency, to ensure that in
post-state-root current block can be safely removed.

Full context:
https://near.zulipchat.com/#narrow/stream/295302-general/topic/.E2.9C.94.20gas.20price.20inconsistency/near/394894144

## Testing

As the change must be no-op, existing tests are enough.
nikurt pushed a commit that referenced this issue Oct 24, 2023
Part of #9618.

In post-state-root, chunk production is moved before block production,
thus we don't have a `Block` during applying chunk anymore. We need to
make sure that all necessary data is still propagated to
`Runtime::apply`.

It should've not have been an error for gas price, as it is taken from
previous block in the general case:
https://github.com/near/nearcore/blob/9c8d0bbec6a71876bb3317db2ade425f0077a184/chain/chain/src/chain.rs#L4120
However, when there is no chunk, there seem to be a very old error - it
is taken from the same block.

Luckily, after `ProtocolFeature::FixApplyChunks` it is not an issue:
when there is no chunk, gas price is not used at all. So I just start
taking it from previous block for consistency, to ensure that in
post-state-root current block can be safely removed.

Full context:
https://near.zulipchat.com/#narrow/stream/295302-general/topic/.E2.9C.94.20gas.20price.20inconsistency/near/394894144

## Testing

As the change must be no-op, existing tests are enough.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-chain Area: Chain, client & related Near Core
Projects
None yet
Development

No branches or pull requests

2 participants