SC: Introduce chain_config_v2 and snapshot_v9 for max_sync_call_depth and max_sync_call_data_size #1257

linh2931 · 2025-03-14T19:26:49Z

This PR introduces a new version of chain_config chain_config_v2 for sync call parameters: max_sync_call_depth and max_sync_call_data_size. They are on chain parameters and require BP agreement for changes.

A new version of snapshot (snapshot_v9) is required to accommodate chain_config_v2.

abieos needs to be updated AntelopeIO/abieos#39

Tests have been updated to verify the new parameters can be read and set.

Resolves #1256

…ata_size

libraries/chain/include/eosio/chain/chain_config.hpp

libraries/chain/chain_config.cpp

spoonincode · 2025-03-14T20:11:04Z

It looks like ship's ABI needs to have chain_config_v2 added. I think we can wait on touching abieos for now though, as long as we don't forget later.

I think snapshot version should be increased to v9 too due to chain_config_v2 too?

libraries/chain/include/eosio/chain/chain_config.hpp

linh2931 · 2025-03-14T20:25:05Z

It looks like ship's ABI needs to have chain_config_v2 added. I think we can wait on touching abieos for now though, as long as we don't forget later.

I think snapshot version should be increased to v9 too due to chain_config_v2 too?

Thanks! Yes, snapshot version needs to be increased to v9. I will change ship and snapshot for now and create an issue for abieos so we won't forget.

spoonincode · 2025-03-15T03:24:21Z

I noticed this will probably need to be changed to use chain_config_v1

spring/libraries/chain/include/eosio/chain/global_property_object.hpp

Lines 59 to 73 in b929322

    
           struct snapshot_global_property_object_v5 { 
        
              // minimum_version and maximum_version refer to chain_snapshot_header 
        
              // version. snapshot_global_property_object_v5 applies to version 5 & 6 
        
              static constexpr uint32_t minimum_version = 5; 
        
              static constexpr uint32_t maximum_version = 6; 
        
              static_assert(chain_snapshot_header::minimum_compatible_version <= maximum_version, 
        
                            "snapshot_global_property_object_v5 is no longer needed"); 
        
              std::optional<block_num_type>       proposed_schedule_block_num; 
        
              producer_authority_schedule         proposed_schedule; 
        
              chain_config                        configuration; 
        
              chain_id_type                       chain_id; 
        
              kv_database_config                  kv_configuration; 
        
              wasm_config                         wasm_configuration; 
        
           };

I'm guessing we're also going to need to create a snapshot_global_property_object_v8 since the current serialized GPO has a chain_config_v1

linh2931 · 2025-03-16T01:01:48Z

It looks like ship's ABI needs to have chain_config_v2 added. I think we can wait on touching abieos for now though, as long as we don't forget later.
I think snapshot version should be increased to v9 too due to chain_config_v2 too?

Thanks! Yes, snapshot version needs to be increased to v9. I will change ship and snapshot for now and create an issue for abieos so we won't forget.

V9 snapshot: 35cee9e

linh2931 · 2025-03-16T01:05:31Z

I noticed this will probably need to be changed to use chain_config_v1

spring/libraries/chain/include/eosio/chain/global_property_object.hpp

Lines 59 to 73 in b929322

struct snapshot_global_property_object_v5 {

// minimum_version and maximum_version refer to chain_snapshot_header

// version. snapshot_global_property_object_v5 applies to version 5 & 6

static constexpr uint32_t minimum_version = 5;

static constexpr uint32_t maximum_version = 6;

static_assert(chain_snapshot_header::minimum_compatible_version <= maximum_version,

"snapshot_global_property_object_v5 is no longer needed");

std::optional<block_num_type> proposed_schedule_block_num;

producer_authority_schedule proposed_schedule;

chain_config configuration;

chain_id_type chain_id;

kv_database_config kv_configuration;

wasm_config wasm_configuration;

};

I'm guessing we're also going to need to create a snapshot_global_property_object_v8 since the current serialized GPO has a chain_config_v1

Thanks. Done in the snapshot commit aa28ece.

It seems the version numbers for Legacy snapshot_global_property_object grows sequentially. I named it snapshot_global_property_object_v6

…sync_call_data_size <= MAX_SIZE_OF_BYTE_ARRAYS

linh2931 · 2025-03-16T19:53:24Z

It looks like ship's ABI needs to have chain_config_v2 added. I think we can wait on touching abieos for now though, as long as we don't forget later.
I think snapshot version should be increased to v9 too due to chain_config_v2 too?

Thanks! Yes, snapshot version needs to be increased to v9. I will change ship and snapshot for now and create an issue for abieos so we won't forget.

V9 snapshot: 35cee9e

SHiP ABI update: 2ffd898

libraries/chain/include/eosio/chain/chain_config.hpp

heifner · 2025-03-17T16:03:38Z

libraries/chain/include/eosio/chain/chain_config.hpp

+
+   friend inline bool operator != ( const chain_config_v2& lhs, const chain_config_v2& rhs ) {
+      return !(lhs == rhs);
+   }


operator!= should not be needed with c++20.

operator!= should not be needed with c++20.

No need to define operator ==() either when = default; is the same thing.

Thanks. Removed.

93f0295

Don't we need the = default still? I think this will skip equality check of v2 members as is

libraries/chain/include/eosio/chain/chain_config.hpp

libraries/chain/include/eosio/chain/global_property_object.hpp

spoonincode · 2025-03-17T18:53:31Z

btw I realize now that AntelopeIO/leap#1770 introduces an inconsistency. The premise is that we shouldn't return wasm_config unless the protocol feature is activated. ok sounds good. But we still always return max_action_return_value_size too -- even if that protocol feature is not enabled. This PR adds two more items returned even when their protocol feature is not activated.

I'm not sure what the right answer is.. we could potentially scrub the get_consensus_parameters reply for any config item related to an unactivated feature. That seems kinda hacky but would at least be consistent with AntelopeIO/leap#1770 and the confusion it solves.

spoonincode · 2025-03-19T18:52:15Z

Also need to update this part of ship,

spring/libraries/state_history/include/eosio/state_history/serialization.hpp

Lines 332 to 333 in 2ffd898

    
           template <typename ST> 
        
           datastream<ST>& operator<<(datastream<ST>& ds, const history_serial_wrapper_stateless<eosio::chain::chain_config>& obj) {

…explicit copy_from_v0 function

spoonincode · 2025-03-19T22:14:58Z

libraries/chain/webassembly/privileged.cpp

      context.db.modify( context.control.get_global_properties(),
         [&]( auto& gprops ) {
-              gprops.configuration = cfg;
+              gprops.configuration.copy_from_v0(cfg);


Why do you need an assignment operator from v0 that leaves v1 and v2 data alone?

This might be why. Does this change technically cause a consensus violation? The old code would leave max_action_return_value_size unchanged but this code will reset it to config::default_max_action_return_value_size. (I don't see any reason one can't call set_blockchain_parameters_packed() after activating ACTION_RETURN_VALUE)

Good catch! Will revert.

I don't think you need to entirely revert but rather copy_from_v0() shouldn't stomp on v1 & v2 items.

I don't think you need to entirely revert

(pending the style question in the other thread)

…ts of v1 fields with assignment operator

linh2931 · 2025-03-20T00:06:53Z

Also need to update this part of ship,

spring/libraries/state_history/include/eosio/state_history/serialization.hpp

Lines 332 to 333 in 2ffd898

template <typename ST>

datastream<ST>& operator<<(datastream<ST>& ds, const history_serial_wrapper_stateless<eosio::chain::chain_config>& obj) {

Thanks. Done.

806fd71

spoonincode · 2025-03-20T13:52:31Z

Also need to update this part of ship,

spring/libraries/state_history/include/eosio/state_history/serialization.hpp

Lines 332 to 333 in 2ffd898

template <typename ST>

datastream<ST>& operator<<(datastream<ST>& ds, const history_serial_wrapper_stateless<eosio::chain::chain_config>& obj) {

Thanks. Done.

806fd71

This part needs to be changed to 2,

spring/libraries/state_history/include/eosio/state_history/serialization.hpp

Line 334 in 806fd71

fc::raw::pack(ds, fc::unsigned_int(1));

(it's manually encoding the variant index).

…iP protocol

…n_config_v2

linh2931 · 2025-03-20T15:09:16Z

ebd7f51

Thank you so much for this! I did not notice that. ebd7f51

I have also added chain_config_v2 to abieos sync_call branch. Unit test test_deltas_global_property_history needs that.

…in variant definition

spoonincode · 2025-03-27T18:57:51Z

libraries/chain/include/eosio/chain/chain_config.hpp

+      // copy v0 fields
+      chain_config_v0::operator= (b);
+
+      // leave v1 and v2 fields alone as changing them might break cosensus


"consensus"

Fixed by 30d934c

linh2931 · 2025-03-28T15:25:41Z

btw I realize now that AntelopeIO/leap#1770 introduces an inconsistency. The premise is that we shouldn't return wasm_config unless the protocol feature is activated. ok sounds good. But we still always return max_action_return_value_size too -- even if that protocol feature is not enabled. This PR adds two more items returned even when their protocol feature is not activated.

I'm not sure what the right answer is.. we could potentially scrub the get_consensus_parameters reply for any config item related to an unactivated feature. That seems kinda hacky but would at least be consistent with AntelopeIO/leap#1770 and the confusion it solves.

I created #1289 to track this.

heifner · 2025-03-28T16:03:14Z

unittests/snapshot_tests.cpp

   auto genesis = eosio::chain::block_log::extract_genesis_state(source_log_dir);
   std::filesystem::create_directories(config.blocks_dir);
   std::filesystem::copy(source_log_dir / "blocks.log", config.blocks_dir / "blocks.log");
   std::filesystem::copy(source_log_dir / "blocks.index", config.blocks_dir / "blocks.index");


The abieos submodule update should be via a PR to abieos. I don't see one.

As the changes were small and I'd to have a quick test, I went ahead making the abieos changes in the sync_call branch. I will make sure all the future changes will be done from a branch created out of the sync_call branch.

heifner

Make sure to update to merged abieos commit.

Introduce chain_config_v2 for max_sync_call_depth and max_sync_call_d…

bdfeb08

…ata_size

linh2931 requested review from heifner and spoonincode March 14, 2025 19:27

linh2931 mentioned this pull request Mar 14, 2025

Need to update setparams for chain_config_v2 parameters introduced by sync call feature VaultaFoundation/system-contracts#197

Open

spoonincode reviewed Mar 14, 2025

View reviewed changes

libraries/chain/include/eosio/chain/chain_config.hpp Outdated Show resolved Hide resolved

libraries/chain/include/eosio/chain/chain_config.hpp Outdated Show resolved Hide resolved

spoonincode reviewed Mar 14, 2025

View reviewed changes

libraries/chain/chain_config.cpp Outdated Show resolved Hide resolved

Update test-data/consensus_blockchain/snapshot for new chain_config

ea867bf

spoonincode reviewed Mar 14, 2025

View reviewed changes

libraries/chain/include/eosio/chain/chain_config.hpp Show resolved Hide resolved

linh2931 added 3 commits March 15, 2025 20:09

Implement Snapshot V9 to support new chain_config_v2

35cee9e

Update snapshot compatible test for snapshot V9

77fcfbb

Update test snapshot for savanna_misc_tests/verify_block_compatibitity

176cedb

linh2931 added 3 commits March 16, 2025 15:15

Correct syntax error in assert text (from no allowed to not allowed)

c77065a

Print out the value of MAX_SIZE_OF_BYTE_ARRAYS for EOS_ASSERT of max_…

318d440

…sync_call_data_size <= MAX_SIZE_OF_BYTE_ARRAYS

Update SHiP ABI file for chain_config_v2

2ffd898

linh2931 mentioned this pull request Mar 16, 2025

Update ship_protocol.hpp and ship.abi.cpp for new chain_config_v2" AntelopeIO/abieos#39

Open

heifner suggested changes Mar 17, 2025

View reviewed changes

spoonincode mentioned this pull request Mar 18, 2025

SC: Implement the core functionalities of sync calls #1251

Merged

linh2931 added 2 commits March 19, 2025 17:07

Remove unnecessary == and != operators

93f0295

Replace a unsafe assignment operator (v1&v2 fields left out) with an …

1890e98

…explicit copy_from_v0 function

spoonincode reviewed Mar 19, 2025

View reviewed changes

linh2931 added 2 commits March 19, 2025 19:56

Do not reset v1 and v2 fields to default and replace manual assignmen…

7a19b34

…ts of v1 fields with assignment operator

Add chain_config_v2 fields to SHiP packing operator

806fd71

add default == operator

fee79f7

linh2931 added 3 commits March 20, 2025 10:53

point to abieos sync_call branch which has new chain_config_v2 for SH…

0296069

…iP protocol

Update test_deltas_global_property_history to use ship_protocol::chai…

4799bfe

…n_config_v2

Update the variant index of chain_config for SHiP serialization

ebd7f51

linh2931 and others added 2 commits March 20, 2025 11:53

Update to point to latest abieos which fixes missing chain_config_v1 …

ddf7fdf

…in variant definition

Merge branch 'call_implementation' into sync_call_limits

e3e4305

Base automatically changed from call_implementation to sync_call March 27, 2025 11:52

spoonincode reviewed Mar 27, 2025

View reviewed changes

spoonincode mentioned this pull request Mar 27, 2025

hide max_action_return_value_size in get_consensus_parameters when feature not activated #1284

Merged

linh2931 and others added 2 commits March 27, 2025 21:23

Merge branch 'sync_call' into sync_call_limits

466c6c2

Correct a typo in a comment

30d934c

linh2931 mentioned this pull request Mar 28, 2025

SC: Do not show chain_config_v2 parameters if SYNC_CALL protocol feature is not activated #1289

Closed

heifner reviewed Mar 28, 2025

View reviewed changes

heifner approved these changes Mar 28, 2025

View reviewed changes

spoonincode approved these changes Mar 31, 2025

View reviewed changes

linh2931 merged commit d390a3f into sync_call Mar 31, 2025
36 checks passed

linh2931 deleted the sync_call_limits branch March 31, 2025 19:31

linh2931 changed the title ~~SC: Introduce chain_config_v2 for max_sync_call_depth and max_sync_call_data_size~~ SC: Introduce chain_config_v2 and snapshot_v9 for max_sync_call_depth and max_sync_call_data_size Apr 3, 2025

SC: Introduce chain_config_v2 and snapshot_v9 for max_sync_call_depth and max_sync_call_data_size #1257

SC: Introduce chain_config_v2 and snapshot_v9 for max_sync_call_depth and max_sync_call_data_size #1257

Uh oh!

Conversation

linh2931 commented Mar 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

spoonincode commented Mar 14, 2025

Uh oh!

Uh oh!

linh2931 commented Mar 14, 2025

Uh oh!

spoonincode commented Mar 15, 2025

Uh oh!

linh2931 commented Mar 16, 2025

Uh oh!

linh2931 commented Mar 16, 2025

Uh oh!

linh2931 commented Mar 16, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greg7mdp Mar 17, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

spoonincode commented Mar 17, 2025

Uh oh!

spoonincode commented Mar 19, 2025

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!

linh2931 commented Mar 20, 2025

Uh oh!

spoonincode commented Mar 20, 2025

Uh oh!

linh2931 commented Mar 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

linh2931 Mar 28, 2025 • 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!

linh2931 commented Mar 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

heifner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

linh2931 commented Mar 14, 2025 •

edited

Loading

greg7mdp Mar 17, 2025 •

edited

Loading

linh2931 Mar 28, 2025 •

edited

Loading