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

Add support for ConsensusParametersVersion::V2 #2188

Conversation

rafal-ch
Copy link
Contributor

Partial implementation of #2133.

Linked Issues/PRs

(FuelLabs/fuel-vm#821)

Description

It adds handling for the updated data struct ConsensusParameters (new field: block_transaction_size_limit). No additional logic around this new parameter is implemented in this PR, it's just a stub for further work.

The new parameter is exposed via GraphQL, example:

Request snippet:

  consensusParameters(version: 0) {
    blockGasLimit
    blockTransactionSizeLimit
  }

Response snippet:

  "consensusParameters": {
    "blockGasLimit": "30000000",
    "blockTransactionSizeLimit": "129024"
  }

Checklist

  • Breaking changes are clearly marked as such in the PR description and changelog
  • New behavior is reflected in tests
  • The specification matches the implemented behavior (link update PR if changes are needed)

Before requesting review

  • I have reviewed the code myself
  • I have created follow-up issues caused by this PR and linked them here

After merging, notify other teams

[Add or remove entries as needed]

@acerone85
Copy link
Contributor

When I run the node and make the following query:

curl -X POST http://localhost:4000/v1/graphql \
-H "Content-Type: application/json" \
-d '{
    "query": "{ consensusParameters(version:0) { version blockGasLimit blockTransactionSizeLimit } }"
}'

I get the following result

{
  "data": {
    "consensusParameters": {
      "version": "V2",
      "blockGasLimit": "30000000",
      "blockTransactionSizeLimit": "129024"
    }
  }
}

Is this the expected result? I would imagine that by querying consensusParameters(version:0) we got still the V1 parameters, and to get the V2 consensus parameters we would need to run a query with consensusParameters(version:1).

Or am I missing something?

@rafal-ch rafal-ch self-assigned this Sep 12, 2024
@rafal-ch
Copy link
Contributor Author

When I run the node and make the following query:

curl -X POST http://localhost:4000/v1/graphql \
-H "Content-Type: application/json" \
-d '{
    "query": "{ consensusParameters(version:0) { version blockGasLimit blockTransactionSizeLimit } }"
}'

I get the following result

{
  "data": {
    "consensusParameters": {
      "version": "V2",
      "blockGasLimit": "30000000",
      "blockTransactionSizeLimit": "129024"
    }
  }
}

Is this the expected result? I would imagine that by querying consensusParameters(version:0) we got still the V1 parameters, and to get the V2 consensus parameters we would need to run a query with consensusParameters(version:1).

Or am I missing something?

My explanation would be that since the chain starts-up at genesis with this change in the chainspec config included, it treats this as "version 0", aka "the first (zero-indexed) and only version known".

I assume that if we had started with the original chainspec containing V1, this V1 would become a version 0. And later if we upgrade the network to V2, this V2 would actually become version 1.

I not super certain this is the expected/correct behavior, though.

@acerone85
Copy link
Contributor

Thanks for explaining. Overall LGTM. I'll just wait for somebody to confirm that the behaviour re the version is correct, and that the breaking changes to fuel-vm are merged. After that I will be happy to approve.

@rafal-ch rafal-ch mentioned this pull request Sep 13, 2024
3 tasks
@rafal-ch rafal-ch marked this pull request as ready for review September 16, 2024 08:48
Copy link
Contributor

@AurelienFT AurelienFT left a comment

Choose a reason for hiding this comment

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

Looks good to me, however I don't feel confortable approving this as I barely understand why each changes has been done.

CHANGELOG.md Outdated
- [2163](https://github.com/FuelLabs/fuel-core/pull/2163): Added runnable task for fetching block committer data.

### Changed

#### Breaking
- [2199](https://github.com/FuelLabs/fuel-core/pull/2199): Applying several breaking changes to the WASM interface from backlog:
Copy link
Contributor

Choose a reason for hiding this comment

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

This description shouldn't be in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, it's not necessarily a part of the feature being implemented in this PR, but the breaking changes was pulled into this branch here and here, so technically, this message belongs here.

I'll most likely not merge this PR before the one which contained the original change is merged first.

@@ -198,6 +198,7 @@ type ConsensusParameters {
feeParams: FeeParameters!
baseAssetId: AssetId!
blockGasLimit: U64!
blockTransactionSizeLimit: U64!
Copy link
Contributor

Choose a reason for hiding this comment

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

When I first read it, I understood that it was the maximum size for a transaction. I think I misunderstood because the word transaction is singular. IMO, it should be plural.

Copy link
Contributor Author

@rafal-ch rafal-ch Sep 17, 2024

Choose a reason for hiding this comment

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

Yes, the name could indeed be slightly improved. However, it's already merged in fuel-vm here.

In the follow-up PR to fuel-core I'll make sure that this is comprehensively documented.

@rafal-ch
Copy link
Contributor Author

Looks good to me, however I don't feel confortable approving this as I barely understand why each changes has been done.

It'll get more clear after #2199 is merged.

@xgreenx xgreenx changed the base branch from master to feature/wasm-interface-breaking-changes September 17, 2024 09:21
Removed `ConsensusParametersVersion::V2` since we can do the change without it=)
…o 2133_block_size_consensus_parameter

# Conflicts:
#	bin/fuel-core/chainspec/local-testnet/state_transition_bytecode.wasm
@xgreenx xgreenx mentioned this pull request Sep 17, 2024
2 tasks
@Dentosal Dentosal merged commit 0ae006e into feature/wasm-interface-breaking-changes Sep 17, 2024
25 of 26 checks passed
@Dentosal Dentosal deleted the 2133_block_size_consensus_parameter branch September 17, 2024 13:58
@Dentosal
Copy link
Member

Sorry about the accidental merge. Feel free to revert if needed.

@xgreenx xgreenx mentioned this pull request Sep 17, 2024
xgreenx added a commit that referenced this pull request Sep 18, 2024
## Version v0.36.0

### Added
- [2135](#2135): Added metrics
logging for number of blocks served over the p2p req/res protocol.
- [2151](#2151): Added
limitations on gas used during dry_run in API.
- [2188](#2188): Added the new
variant `V2` for the `ConsensusParameters` which contains the new
`block_transaction_size_limit` parameter.
- [2163](#2163): Added
runnable task for fetching block committer data.
- [2204](#2204): Added
`dnsaddr` resolution for TLD without suffixes.

### Changed

#### Breaking
- [2199](#2199): Applying
several breaking changes to the WASM interface from backlog:
- Get the module to execute WASM byte code from the storage first, an
fallback to the built-in version in the case of the
`FUEL_ALWAYS_USE_WASM`.
- Added `host_v1` with a new `peek_next_txs_size` method, that accepts
`tx_number_limit` and `size_limit`.
- Added new variant of the return type to pass the validation result. It
removes block serialization and deserialization and should improve
performance.
- Added a V1 execution result type that uses `JSONError` instead of
postcard serialized error. It adds flexibility of how variants of the
error can be managed. More information about it in
FuelLabs/fuel-vm#797. The change also moves
`TooManyOutputs` error to the top. It shows that `JSONError` works as
expected.
- [2145](#2145): feat:
Introduce time port in PoA service.
- [2155](#2155): Added trait
declaration for block committer data
- [2142](#2142): Added
benchmarks for varied forms of db lookups to assist in optimizations.
- [2158](#2158): Log the
public address of the signing key, if it is specified
- [2188](#2188): Upgraded the
`fuel-vm` to `0.57.0`. More information in the
[release](https://github.com/FuelLabs/fuel-vm/releases/tag/v0.57.0).

## What's Changed
* chore(p2p_service): add metrics for number of blocks requested over
p2p req/res protocol by @rymnc in
#2135
* Weekly `cargo update` by @github-actions in
#2149
* Debug V1 algorightm and use more realistic values in gas price
analysis by @MitchTurner in
#2129
* feat(gas_price_service): include trait declaration for block committer
data by @rymnc in #2155
* Convert gas price analysis tool to CLI by @MitchTurner in
#2156
* chore: add benchmarks for varied forms of lookups by @rymnc in
#2142
* Add label nochangelog on weekly cargo update by @AurelienFT in
#2152
* Log consensus-key signer address if specified by @acerone85 in
#2158
* chore(rocks_db): move ShallowTempDir to benches crate by @rymnc in
#2168
* chore(benches): conditional dropping of databases in benchmarks by
@rymnc in #2170
* feat: Introduce time port in PoA service by @netrome in
#2145
* Get DA costs from predefined data by @MitchTurner in
#2157
* chore(shallow_temp_dir): panic if not panicking by @rymnc in
#2172
* chore: Add initial CODEOWNERS file by @netrome in
#2179
* Weekly `cargo update` by @github-actions in
#2177
* fix(db_lookup_times): rework core logic of benchmark by @rymnc in
#2159
* Add verification on transaction dry_run that they don't spend more
than block gas limit by @AurelienFT in
#2151
* bug: fix algorithm overflow issues by @MitchTurner in
#2173
* feat(gas_price_service): create runnable task for expensive background
polling for da metadata by @rymnc in
#2163
* Weekly `cargo update` by @github-actions in
#2197
* Fix bug with gas price factor in V1 algorithm by @MitchTurner in
#2201
* Applying several breaking changes to the WASM interface from backlog
by @xgreenx in #2199
* chore(p2p): dnsaddr recursive resolution by @rymnc in
#2204

## New Contributors
* @acerone85 made their first contribution in
#2158

**Full Changelog**:
v0.35.0...v0.36.0
xgreenx added a commit that referenced this pull request Sep 27, 2024
Closes #2133

## Linked Issues/PRs
Built on top of: #2188

## Description
This PR adds the handling and enforcement of then new consensus
parameter added [here](#2188).

Changes summary:
- `TransactionsSource` and `transaction_selector` use the new
`block_transaction_size_limit` parameter.
- `ExecutionData` contains `used_size` in addition to `used_gas`
- Introduces integration tests covering the expected behavior
- E2E tests affected by the new limit are updated

## Checklist
- [X] Breaking changes are clearly marked as such in the PR description
and changelog
- [X] New behavior is reflected in tests

### Before requesting review
- [X] I have reviewed the code myself

---------

Co-authored-by: green <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants