Change submit transaction spec_version and transaction_version query from chain

[fewensa]

After doing this, we don’t need to upgrade bridger after a chain runtime is upgraded.

[svyatonik]

But it also means that if something changes in the bridge pallets (or in some other involved pallets) the relayer will keep creating old transactions, which may have different meaning in new spec_version or even different SCALE format in new transaction_version. In worst case relayer will keep submitting invalid transactions. which may even lead to funds loss if they are accepted to the pool. So in my opinion the this step requires manual approval => we even have spec_version guards (which abort relay process) enabled for some bridges. WDYT about it?

If you still need this patch, then I suggest to make it optional - e.g. probably change TransactionSignScheme trait to accept Client<Self::Chain> and read all required fields in this implementation for your chain. Alternatively - you may consider adding some CLI options for that.

[fewensa]

I think sign_transaction accept Client<Self::Chain> isn't a good way.

for example.

parity-bridges-common/relays/bin-substrate/src/chains/kusama_messages_to_polkadot.rs

Lines 64 to 96 in 80c9429

	/// Update Polkadot -> Kusama conversion rate, stored in Kusama runtime storage.
	pub(crate) async fn update_polkadot_to_kusama_conversion_rate(
	client: Client<Kusama>,
	signer: <Kusama as TransactionSignScheme>::AccountKeyPair,
	updated_rate: f64,
	) -> anyhow::Result<()> {
	let genesis_hash = *client.genesis_hash();
	let signer_id = (*signer.public().as_array_ref()).into();
	client
	.submit_signed_extrinsic(signer_id, move |_, transaction_nonce| {
	Bytes(
	Kusama::sign_transaction(
	genesis_hash,
	&signer,
	relay_substrate_client::TransactionEra::immortal(),
	UnsignedTransaction::new(
	relay_kusama_client::runtime::Call::BridgePolkadotMessages(
	relay_kusama_client::runtime::BridgePolkadotMessagesCall::update_pallet_parameter(
	relay_kusama_client::runtime::BridgePolkadotMessagesParameter::PolkadotToKusamaConversionRate(
	sp_runtime::FixedU128::from_float(updated_rate),
	)
	)
	),
	transaction_nonce,
	),
	)
	.encode(),
	)
	})
	.await
	.map(drop)
	.map_err(|err| anyhow::format_err!("{:?}", err))
	}

the client can not move to prepare_extrinsic function. because it used by client.submit_signed_extrinsic we need clone this.

I think keeping the current code is better. but your suggestions are good. So I want add

#[derive(Clone, Debug)]
pub enum ChainRuntimeVersion {
	Auto,
	Custom(u32, u32),
	Bundle,
}

into relay_substrate_client::Client

But i won't let sign_transaction accept Client<Self::Chain>. many things will be changed. like sign_transaction must be change to async function and submit_signed_extrinsic must accept async prepare_extrinsic function.
What do you think?

[svyatonik]

Client is lightweight + cloneable, so it shouldn't be a big problem. But the second option is ok to me too if it seems easier to you :)

[fewensa]

Yes. I think the second option is easy to use. if not we need change

parity-bridges-common/relays/client-substrate/src/client.rs

Line 347 in 80c9429

prepare_extrinsic: impl FnOnce(HeaderIdOf<C>, C::Index) -> Bytes + Send + 'static,

to

prepare_extrinsic: impl FnOnce(HeaderIdOf<C>, C::Index) -> BoxFuture<relay_substrate_client::Result<Bytes>> + Send + 'static,

This is difficult to modify, and there are many changes.

[fewensa]

I'm not sure it is correct.

[svyatonik]

Could you, please, use rialto_parachain_runtime::VERSION.spec_version here?

[fewensa]

It fixed here 91c9d51 please check it

[fewensa]

I'm not found what reason about ci error for continuous-integration/gitlab-spellcheck

[svyatonik]

I'm not found what reason about ci error for continuous-integration/gitlab-spellcheck

Here's my local output:

Output

error: spellcheck(Hunspell)
    --> /home/svyatonik/dev/parity-bridges-common/relays/bin-substrate/src/cli/mod.rs:375
     |
 375 |  Custom spec_version and transaction_version
     |         ^^^^^^^^^^^^
     | - perversion or interspersion
     |
     |   Possible spelling mistake found.

error: spellcheck(Hunspell)
    --> /home/svyatonik/dev/parity-bridges-common/relays/bin-substrate/src/cli/mod.rs:375
     |
 375 |  Custom spec_version and transaction_version
     |                          ^^^^^^^^^^^^^^^^^^^
     | - transubstantiation
     |
     |   Possible spelling mistake found.

error: spellcheck(Hunspell)
    --> /home/svyatonik/dev/parity-bridges-common/relays/bin-substrate/src/cli/mod.rs:410
     |
 410 | The custom sepc_version for chain 
     |            ^^^^^^^^^^^^
     | - perversion
     |
     |   Possible spelling mistake found.

error: spellcheck(Hunspell)
    --> /home/svyatonik/dev/parity-bridges-common/relays/bin-substrate/src/cli/mod.rs:413
     |
 413 | The custom transaction_version for chain 
     |            ^^^^^^^^^^^^^^^^^^^
     | - transubstantiation
     |
     |   Possible spelling mistake found.

[2021-12-15T07:20:45Z INFO  cargo_spellcheck::action] ❌ /home/svyatonik/dev/parity-bridges-common/relays/bin-substrate/src/cli/mod.rs : 4
error: spellcheck(Hunspell)
   --> /home/svyatonik/dev/parity-bridges-common/relays/client-substrate/src/client.rs:69
    |
 69 |  the first is spec_version
    |               ^^^^^^^^^^^^
    | - perversion or interspersion
    |
    |   Possible spelling mistake found.

error: spellcheck(Hunspell)
   --> /home/svyatonik/dev/parity-bridges-common/relays/client-substrate/src/client.rs:70
    |
 70 |  the second is transaction_version
    |                ^^^^^^^^^^^^^^^^^^^
    | - transubstantiation
    |
    |   Possible spelling mistake found.

The idea is that (apart of sepc_verstion typo) all variable/function names should be enclosed in backquoutes.

We'll definitely need to do something with spellcheck logs at CI - I'll create an issue for that.

[svyatonik]

Looks good. Left couple of suggestions + spellckech fix is required

[svyatonik]

Could we replace these two parameters with Option<RuntimeVersion> here? Option because for some chains (where we won't submit any transaction) the version may be unknown (so it shall be treated as ChainRuntimeVersion::Auto). And using RuntimeVersion would allow us replace four consts with two, where to_client() is called. We'll probably move VERSION constants to Chain trait later => remove these consts at all.

[svyatonik]

Last q - is there any specific reason you have decided to use two Option<u32>arguments instead of single Option<RuntimeVersion> as I've suggested here? If not, we'll change this in near future

[fewensa]

Sorry, I missing here. Let me think about it. Your suggestions are better. I can change it. Please wait.

[fewensa]

e607a37

[fewensa]

Done

[fewensa]

I'm not sure what happened in CI

  --> /usr/local/cargo/registry/src/github.com-1ecc6299db9ec823/rsix-0.23.9/src/imp/linux_raw/arch/inline/x86_64.rs:73:5
   |
73 |     asm!(
   |     ^^^
   |
   = note: consider importing one of these items:
           std::arch::asm
           core::arch::asm
error: cannot find macro `asm` in this scope

It's ok for my local computer

[svyatonik]

I'm not sure what happened in CI

  --> /usr/local/cargo/registry/src/github.com-1ecc6299db9ec823/rsix-0.23.9/src/imp/linux_raw/arch/inline/x86_64.rs:73:5
   |
73 |     asm!(
   |     ^^^
   |
   = note: consider importing one of these items:
           std::arch::asm
           core::arch::asm
error: cannot find macro `asm` in this scope

It's ok for my local computer

Should be something with todays nightly. Will look today

UPD: #1255

[svyatonik]

CI issue should be fixed now. Could you, please, push some dummy commit (e.g. some change + immediate git revert) to restart CI (i have no rights for that :) ). There's also question from my side && it could be merged then

[svyatonik]

Perfect, thank you!