feat: rename contract_compile gas parameter

[jakmeier]

The compilation step happens during deployment and is thus charged as
part of DeployContractAction. The fee in question is charged when
loading pre-compiled code into the VM for execution. Therefore, it
should be named as such.

This is not a protocol change. It does not change how much gas is
charged nor when it is charged. The only observable difference is the
names used in the gas profiles.

[jakmeier]

A small but significant change to service #5962.

I created this as a separate PR to make sure we can consider carefully that this does not have any unwanted side-effects at some distant place.

[frol]

@matklad @bowenwang1996 I am not sure how to proceed now, but there is quite unfortunate API breaking change.

I created this as a separate PR to make sure we can consider carefully that this does not have any unwanted side-effects at some distant place.

This actually broke JSON RPC API (cc @miraclx @khorolets) compatibility since previously there was contract_compile_base field in EXPERIMENTAL_protocol_config API, and now there is no such field (new fields do not cause any problems on JSON deserialization, but missing fields do).

Apps that use near-jsonrpc-client-rs (which uses near-primitives-core published on creates.io) cannot use the EXPERIMENTAL_protocol_config API on testnet at the moment (mainnet doesn't have this change yet):

RpcError: RpcError { error_struct: Some(RequestValidationError(ParseError { error_message: "Failed to parse: Error(\"missing field `contract_compile_base`\", line: 0, column: 0)" })), code: -32700, message: "Parse error", data: Some(String("Failed to parse: Error(\"missing field `contract_compile_base`\", line: 0, column: 0)")) }

I also believe it broke the tx JSON RPC API where gas_profile is the list of costs where CONTRACT_COMPILE_BASE identifier was used:

[matklad]

I also believe it broke the tx JSON RPC API where gas_profile is the list of costs where CONTRACT_COMPILE_BASE identifier was used:

For gas profile, the JSON type is deliberately Vec<(String, String, Gas)>, to make sure we don't hit such kinds of problems.

nearcore/core/primitives/src/views.rs

Lines 1107 to 1122 in 8d2a9fe

	#[cfg_attr(feature = "deepsize_feature", derive(deepsize::DeepSizeOf))]
	#[derive(BorshSerialize, BorshDeserialize, Serialize, Deserialize, PartialEq, Clone, Eq, Debug)]
	pub struct CostGasUsed {
	pub cost_category: String,
	pub cost: String,
	#[serde(with = "u64_dec_format")]
	pub gas_used: Gas,
	}
	#[cfg_attr(feature = "deepsize_feature", derive(deepsize::DeepSizeOf))]
	#[derive(BorshSerialize, BorshDeserialize, Serialize, Deserialize, PartialEq, Clone, Eq, Debug)]
	pub struct ExecutionMetadataView {
	pub version: u32,
	pub gas_profile: Option<Vec<CostGasUsed>>,
	}

For the config, yeah, that's problematic...

Am I correct that, as this is a somewhat old change, the code is sufficiently deployed that we need to fix that on the client-side, rather than on the server side? That is, on server-side solution would be to just slap serde(rename) on those fields, but would that actually solve anything?

If the answer to above is "yes", then what we could do is slapping serde(default) instead, so that new version of JSON RPC client can read both old and new versions of the config?

What action item do we have to fix the immediate problem?

---

I would also like to see some long-term plan here. It's indeed quite suboptimal that a rename in the guts of nearcore can break API clients without us noticing that for more than a month! I think this is a result of three somewhat related, but different issues:

• we don't have a nearcore-independent crate which specifies our JSON API.
• we don't have tests which check JSON API stability
• for this specific API endpoint, the de-facto JSON format we use leaks internal impl details and is not future-proofed. There's no RuntimeConfigView. Curiously, I believe @jakmeier's work on parameters might help here?

[jakmeier]

Very unfortunate. The change is in 1.26, so it is already in the pipeline to go on mainnet.
So I guess an updated release of primitives-core and near-jsonrpc-client-rs is all we can do now, right?

To prevent the same thing in the future, yeah if we want to change the API endpoint it could potentially help that parameters are now flat and enumeratable. That is, we could report a list of parameters and value pairs instead of the JSON. But I am not sure if this is desirable from the users' perspective.

Regardless, tests for RPC endpoints stability still sound like a good idea.

[frol]

I feel we can still issue a hot-fix for the breaking change as 1.26.1, can't we? (only RPC nodes should be updated, so validators don't need to update) @bowenwang1996 @mm-near

For gas profile, the JSON type is deliberately Vec<(String, String, Gas)>, to make sure we don't hit such kinds of problems.

@matklad It only solves the serialization/deserialization, but technically if someone relied on that specific metric, they will get a non-documented breaking change.

I would also like to see some long-term plan here. It's indeed quite suboptimal that a rename in the guts of nearcore can break API clients without us noticing that for more than a month! I think this is a result of three somewhat related, but different issues

We should finally extract all the serialization/deserialization from allover the places and have a single location for JSON RPC #5516, and similarly we will have it addressed for Indexer Framework.

[bowenwang1996]

@frol my two cents:

• It is unfortunate that this has happened and we should have tests to prevent this from happening again in the future
• The endpoint is EXPERIMENTAL_* and I believe it means that there is no guarantee on backward compatibility.
• Given that nearcore has already been released and many node operators have updated, it seems to me that it is easier to fix on the client side.

[frol]

Given that nearcore has already been released and many node operators have updated, it seems to me that it is easier to fix on the client side.

@bowenwang1996 first of all, it is never "easier to fix on the client side" because there are unknown number of clients while there is known number of nodes. Second, the fix will anyway land on nearcore repo since "client side" (at least near-jsonrpc-client-rs) is based on nearcore primitive crates, so we will need Node team to take care of it; can Node team take #5516 any time soon?

[matklad]

@frol #6844

[bowenwang1996]

@frol yeah that makes sense.

so we will need Node team to take care of it; can Node team take #5516 any time soon?

Unfortunately not this quarter