NDRS-157: get block limits from the chainspec by Fraser999 · Pull Request #113 · casper-network/casper-node

Fraser999 · 2020-07-23T14:20:28Z

This PR changes the deploy buffer to retrieve deploy configuration data from the chainspec rather than relying on having the values passed in.

After discussion, it has also required two new fields to be added to the DeployConfig portion of the chainspec:

block_gas_limit: u64 defaulted to 10,000,000,000,000
block_max_deploy_count: u32 defaulted to 3

Default impls have been added for chainspec::DeployConfig and chainspec::HighwayConfig. These were only implemented for their chainspec::config counterparts previously, but implementing them for the actual chainspec types makes for simpler testing. The config counterparts now have their Defaults implemented via the default values of the main chainspec structs.

There are also a few unrelated fixes to the Makefile included in this PR.

goral09 · 2020-07-23T14:22:05Z

There's a lot of going on in here, things that are unrelated to each other (changes to makefile, adding defaults, adding new config params to the chainspec etc) and still only one commit :)

node/src/components/chainspec_handler/chainspec.rs

node/src/effect/announcements.rs

Fraser999 · 2020-07-23T14:25:45Z

Yeah - sorry. It's down to the order in which the task evolved. I was half way through the changes to the deploy buffer when I found out I needed to change the chainspec. I guess at that point I should have stashed my changes and just worked on the chainspec since that could be a standalone commit.

I have no excuse for the Makefile being bundled in here except habit.

Let me see if I can split these up.

goral09 · 2020-07-23T14:26:39Z

resources/local/chainspec.toml

+# The upper limit of total gas of all deploys in a block.
+block_gas_limit = 10000000000000
+# The maximum number of deploys permitted in a single block.
+block_max_deploy_count = 3


This shouldn't be part of chainspec since it's going to be removed. We're not validating it either.

Ah - I must have misunderstood the conversation. I'll remove it.

goral09 · 2020-07-23T14:27:41Z

Let me see if I can split these up.

Don't do it now. I have already started reviewing and you would have to force push.

goral09 · 2020-07-23T14:32:39Z

node/src/components/deploy_buffer.rs

+    where
+        REv: From<StorageRequest<Storage>> + Send,
+    {
+        // TODO - should the current protocol version be passed in here?


Yes, it should probably come from the consensus together with the request.

I'd prefer if consensus only passed in the block height and era number, from which it should be possible to compute the protocol version?

I don't know, maybe. That's what we're really doing in Scala – consensus provides block height (not even era number which might have been a mistake really 🤔 ) and there's a component that returns a proper configuration.

Sure, decoupling consensus from the idea of chainspec and upgrades makes sense. It would have to be initialized with the proper set of configurations itself.

I'm not clear on how I'd derive the protocol version from a given block height and era number yet, but since I'm not even getting those passed here, how about if I add a protocol_version field to BlockContext which is always set to 1.0.0 for now? Then if consensus wants to change to pass block height and era, we'd have to update this call site to accommodate it.

You wouldn't be doing it – chainspec handler (or whatever the name is of the component that is initialized with the chainspec files) has to know the mapping between block height and the chainspec version.

OK - I'll leave this as a TODO for now.

afck · 2020-07-23T14:35:59Z

node/src/components/deploy_buffer.rs

+        let version = Version::from((1, 0, 0));
+        effect_builder
+            .get_chainspec(version)
+            .event(move |result| Event::GetChainspecResult {


So we're requesting the chainspec every time via an effect? That seems convoluted: Some components will probably use some value from the chainspec every time they handle an event.
I think the consensus component currently gets initialized with some information from the chain specs, and stores it internally — maybe also not a good idea, since it duplicates the data?
I'm not sure what's the best approach to this, but it would be good to have a solution that gives all components quick (and easy!) access to the chainspec at all times.

In Scala node we were keeping chainspecs, keyed by protocol version, in memory and every time a block was about to be created (or was being validated) we would fetch that chainspec and use.

My initial thought was to cache copies of the chainspec (or relevant parts) in components which needed it too in order to simplify things, but @EdHastingsCasperLabs made a good point about these needing to be kept synchronized with the definitive one held by the storage component.

Changing from that pattern is probably worthy of a discussion outside of this PR?

goral09 · 2020-07-23T14:37:04Z

node/src/components/deploy_buffer.rs

+    #[from]
+    Request(DeployBufferRequest),
+    /// A new deploy should be buffered.
+    Buffer {


I'm still surprised that we're treating deploy storage and deploy buffer separately – i.e. latter doesn't wrap the former but maybe I'm wrong.

goral09

No real blockers.

113: Bump Version for 1.4.7 r=sacherjj a=sacherjj Bumped casper-node to 1.4.7, EE to 2.0.1, and assembly script to 1.4.7 (for CI requirements) Co-authored-by: Joe Sacher <321623+sacherjj@users.noreply.github.com> Co-authored-by: Fraser Hutchison <fraser@casperlabs.io>

3302: Merge 1.4.8 to dev r=mpapierski a=mpapierski This PR merges the 1.4.8 bugfix release into dev by cherry-picking relevant fixes from the history of the v1.4.8 release. A complete history of v1.4.8 release as compared to dev: dev...v1.4.8 Commits cherry-picked in order starting from `Clippy fixes with recent stable` (a369e37). | Message | Git SHA on v1.4.8 | Cherry-picked | | --- | --- | --- | `(Merge #115, 2022-08-14)` | b94c4f7 | merge commit | `(version bump casper-node and assembly script, 2022-08-12)` | 389dddc | 696206e | `(Merge pull request #114 from casper-network/regression-20220727-part2, 2022-08-12)` | 281061f | merge commit | `(Add changelog and docs., 2022-08-12)` | 01999db | a301241 | `(Shuffle ensure_* functions in order, 2022-08-12)` | 3677f9d | 99bfdd9 | `(Simplify checking algorithm, 2022-08-12)` | 6658738 | 5a7d1bd | `(Fix clippy issues., 2022-08-12)` | 0ee97fc | empty commit | `(Add checks to avoid panics in the gas accounting, 2022-08-11)` | 4340204 | 3881820 | `(Add regression tests for gas counter injector., 2022-08-11)` | e710b8e | 09f023a | `(Restrict access to non-declared globals., 2022-08-10)` | 7944881 | a2ca3dc | `(Regression tests for global set/get access., 2022-08-10)` | 843cf53 | 5432145 | `(Merge #113, 2022-08-03)` | 298dfaa | merge commit | `(further updates required for version bumps, 2022-08-03)` | 9129165 | 5c07b4e | `(Bumping EE, casper-node and assembly script (to keep from erroring on publish)., 2022-08-02)` | 55a413d | 6823aa6 | `(Merge pull request #110 from casper-network/regression-20220727, 2022-08-02)` | 17a39d8 | | `(Fix backwards compatibility, 2022-08-02)` | 476a236 | 1ac4e8c | `(Apply suggestions from code review, 2022-08-02)` | 7ae3f1d | 11c0f95 | `(Rewrite for clarity as `@goral09` suggested, 2022-08-01)` | 7b84dfa | 1de2f62 | `(Ensure internal gas function cannot be imported., 2022-07-29)` | 31fe6b3 | 822bea0 | `(Ensure functions max param size., 2022-07-29)` | ea0f05c | 8f2dff4 | `(Ensure module doesnt declare more than 256 globals, 2022-07-29)` | 20a667a | 496f5a8 | `(Limit the size of a br_table to 256 elements., 2022-07-29)` | 3d51997 | 83023e1 | `(Apply review comments, 2022-07-29)` | e562a5c | 5ec9af7 | `(Apply review comments, 2022-07-28)` | 223d861 | f064a36 | `(Lower the table size to 4k, 2022-07-28)` | 438f56e | 66b3e9c | `(Fix the issue by imposing a limit for table size., 2022-07-27)` | 7dc6dcb | bec2292 | `(Add regression test to trigger the issue., 2022-07-27)` | 1eafe74 | c31d448 | `(Clippy fixes with recent stable, 2022-07-27)` | a369e37 | effectively empty commit | Co-authored-by: Michał Papierski <michal@casperlabs.io> Co-authored-by: Joe Sacher <321623+sacherjj@users.noreply.github.com> Co-authored-by: Fraser Hutchison <fraser@casperlabs.io>

NDRS-157: get block limits from the chainspec

dcdfb16

Fraser999 requested review from EdHastingsCasperAssociation, afck, fizyk20, goral09, marc-casperlabs and mpapierski July 23, 2020 14:20

goral09 reviewed Jul 23, 2020

View reviewed changes

node/src/components/chainspec_handler/chainspec.rs Show resolved Hide resolved

goral09 reviewed Jul 23, 2020

View reviewed changes

node/src/effect/announcements.rs Show resolved Hide resolved

goral09 reviewed Jul 23, 2020

View reviewed changes

afck reviewed Jul 23, 2020

View reviewed changes

goral09 reviewed Jul 23, 2020

View reviewed changes

goral09 approved these changes Jul 23, 2020

View reviewed changes

NDRS-157: remove block_max_deploy_count from DeployConfig

34f58e0

afck approved these changes Jul 23, 2020

View reviewed changes

NDRS_157: add block_max_deploy_count to NodeConfig

9d92381

Fraser999 merged commit 64cf778 into casper-network:master Jul 23, 2020

Fraser999 deleted the NDRS-157-deploy-buffer branch July 23, 2020 18:39

rafal-ch pushed a commit that referenced this pull request Sep 11, 2024

only check for duplicate utxo_id if input isn't a contract (#113)

a0db6ca

rafal-ch pushed a commit that referenced this pull request Sep 11, 2024

Implement unsafe and wrapping ALU flags (#113)

d37969a

Conversation

Fraser999 commented Jul 23, 2020

Uh oh!

goral09 commented Jul 23, 2020

Uh oh!

Uh oh!

Uh oh!

Fraser999 commented Jul 23, 2020

Uh oh!

goral09 Jul 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goral09 commented Jul 23, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goral09 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

goral09 Jul 23, 2020 •

edited

Loading