Remove &mut constraint from BlockImport API

[cecton]

Use case

I'm trying to make an integration test in cumulus where I run multiple full nodes and I need to register a block. For that I would like to use the import method of the trait ClientBlockImportExt which depends on BlockImport. If I can't, I want to be able to use at least the method import_block from BlockImport.

In polkadot I have extended the trait PolkadotClient to impl BlockImport. But no matter what I put there, I couldn't get it working.

The error

error[E0599]: no method named `import` found for struct `alloc::sync::Arc<impl polkadot_service::client::PolkadotClient<sp_runtime::generic::block::Block<sp_runtime::generic::header::Header<u32, sp_runtime::traits::BlakeTwo256>, sp_runtime::OpaqueExtrinsic>, sc_client_db::Backend<sp_runtime::generic::block::Block<sp_runtime::generic::header::Header<u32, sp_runtime::traits::BlakeTwo256>, sp_runtime::OpaqueExtrinsic>>, polkadot_test_runtime::RuntimeApi>>` in the current scope
   --> polkadot-test-service/tests/build-blocks.rs:47:16
    |
47  |           alice.client.import((), ()).unwrap(); // TODO
    |                        ^^^^^^ method not found in `alloc::sync::Arc<impl polkadot_service::client::PolkadotClient<sp_runtime::generic::block::Block<sp_runtime::generic::header::Header<u32, sp_runtime::traits::BlakeTwo256>, sp_runtime::OpaqueExtrinsic>, sc_client_db::Backend<sp_runtime::generic::block::Block<sp_runtime::generic::header::Header<u32, sp_runtime::traits::BlakeTwo256>, sp_runtime::OpaqueExtrinsic>>, polkadot_test_runtime::RuntimeApi>>`
    |
    = note: the method `import` exists but the following trait bounds were not satisfied:
            `<&'r impl polkadot_service::client::PolkadotClient<sp_runtime::generic::block::Block<sp_runtime::generic::header::Header<u32, sp_runtime::traits::BlakeTwo256>, sp_runtime::OpaqueExtrinsic>, sc_client_db::Backend<sp_runtime::generic::block::Block<sp_runtime::generic::header::Header<u32, sp_runtime::traits::BlakeTwo256>, sp_runtime::OpaqueExtrinsic>>, polkadot_test_runtime::RuntimeApi> as sp_consensus::block_import::BlockImport<_>>::Error = sp_consensus::error::Error`
            which is required by `alloc::sync::Arc<impl polkadot_service::client::PolkadotClient<sp_runtime::generic::block::Block<sp_runtime::generic::header::Header<u32, sp_runtime::traits::BlakeTwo256>, sp_runtime::OpaqueExtrinsic>, sc_client_db::Backend<sp_runtime::generic::block::Block<sp_runtime::generic::header::Header<u32, sp_runtime::traits::BlakeTwo256>, sp_runtime::OpaqueExtrinsic>>, polkadot_test_runtime::RuntimeApi>>: substrate_test_client::client_ext::ClientBlockImportExt<_>`
            `<&'r impl polkadot_service::client::PolkadotClient<sp_runtime::generic::block::Block<sp_runtime::generic::header::Header<u32, sp_runtime::traits::BlakeTwo256>, sp_runtime::OpaqueExtrinsic>, sc_client_db::Backend<sp_runtime::generic::block::Block<sp_runtime::generic::header::Header<u32, sp_runtime::traits::BlakeTwo256>, sp_runtime::OpaqueExtrinsic>>, polkadot_test_runtime::RuntimeApi> as sp_consensus::block_import::BlockImport<_>>::Transaction = _`
            which is required by `alloc::sync::Arc<impl polkadot_service::client::PolkadotClient<sp_runtime::generic::block::Block<sp_runtime::generic::header::Header<u32, sp_runtime::traits::BlakeTwo256>, sp_runtime::OpaqueExtrinsic>, sc_client_db::Backend<sp_runtime::generic::block::Block<sp_runtime::generic::header::Header<u32, sp_runtime::traits::BlakeTwo256>, sp_runtime::OpaqueExtrinsic>>, polkadot_test_runtime::RuntimeApi>>: substrate_test_client::client_ext::ClientBlockImportExt<_>`
            `&'r impl polkadot_service::client::PolkadotClient<sp_runtime::generic::block::Block<sp_runtime::generic::header::Header<u32, sp_runtime::traits::BlakeTwo256>, sp_runtime::OpaqueExtrinsic>, sc_client_db::Backend<sp_runtime::generic::block::Block<sp_runtime::generic::header::Header<u32, sp_runtime::traits::BlakeTwo256>, sp_runtime::OpaqueExtrinsic>>, polkadot_test_runtime::RuntimeApi>: sp_consensus::block_import::BlockImport<_>`
            which is required by `alloc::sync::Arc<impl polkadot_service::client::PolkadotClient<sp_runtime::generic::block::Block<sp_runtime::generic::header::Header<u32, sp_runtime::traits::BlakeTwo256>, sp_runtime::OpaqueExtrinsic>, sc_client_db::Backend<sp_runtime::generic::block::Block<sp_runtime::generic::header::Header<u32, sp_runtime::traits::BlakeTwo256>, sp_runtime::OpaqueExtrinsic>>, polkadot_test_runtime::RuntimeApi>>: substrate_test_client::client_ext::ClientBlockImportExt<_>

(The particular error come from this code)

This problem occurs because the API of block import requires that self is given as mutable reference (&mut self) and you can't access it if it's wrapped in an Arc (that's because Arc does not implement DerefMut but only Deref). Unfortunately, the client is, afaik, always wrapped in an Arc except in our tests.

Solution proposed

The mutable reference requirement is actually not needed. I could remove it without breaking anything. So, to solve this problem I replaced all the mutable reference to self in all methods of the traits BlockImport and ClientBlockImportExt. Everything works just fine but I had to do a lot of changes everywhere.

I also changed some tests so they use an Arc<Client> instead of a simple Client. Often they were going to be wrapped in an Arc anyway.

Companion PR: paritytech/polkadot#1319

[andresilva]

Can you point me to the original code you show in the description for why this is needed?

[cecton]

@andresilva I just update the description: here is the code.

[seunlanlege]

So I'm not entirely sure why the API required a &mut reference in the first place, maybe someone else can explain.

[andresilva]

@seunlanlege #3058
This was explicitly changed to take &mut self sometime ago and I'm not sure it makes sense to go back. The description of the issue doesn't make a lot of sense since we use Arc<Client> all over the substrate codebase and we have no issues with importing blocks.

[cecton]

we use Arc<Client> all over the substrate codebase and we have no issues with importing blocks.

@andresilva Yes and no:

• We do use Arc<Client> all over the place for running a node but we don't import block, I couldn't find an example of code using it (I checked substrate and polkadot) and I believe it is technically impossible: how would you get a mutable reference of something in an Arc??
• The only places where these implementations are used are with test client but there is a specific impl for Client which makes it work (also because those tests don't use Arc<Client> but just Client)

[andresilva]

https://github.com/paritytech/substrate/blob/master/client/finality-grandpa/src/import.rs#L59
https://github.com/paritytech/substrate/blob/master/client/finality-grandpa/src/import.rs#L440

And also from polkadot using test client: https://github.com/paritytech/polkadot/blob/master/node/service/src/grandpa_support.rs#L249

I think you need to look into the ClientBlockImportExt trait since that's what's giving you troubles in your test.

[cecton]

I think you need to look into the ClientBlockImportExt trait since that's what's giving you troubles in your test.

I just can't use any of them, I tried. I already spent hours and hours trying everything and I just can't use those traits. If anyone has a solution I would be very happy too and will probably close this PR.

Also you're not explaining why this has been converted an an &mut. I would really like to know that to understand the big picture.

Right now it looks like it's the right thing to do because all the other impl for PolkadotClient look very alike (without the `for<'r> &'r Client: syntax). As you can see here.

[tomaka]

Before modifying a trait to take &self instead of &mut self, consider that you can implement the trait in question on Arc<T> rather than T. In particular, you can do impl BlockImport for Arc<Client> and keep the &mut self.

Also you're not explaining why this has been converted an an &mut. I would really like to know that to understand the big picture.

The reason is that, for any non-idempotent method, &self is always strictly worse than &mut self. It means "anything can modify the state of the underlying object at any time", which makes the logic way more difficult to understand.

[cecton]

consider that you can implement the trait in question on Arc<T> rather than T. In particular, you can do impl BlockImport for Arc<Client> and keep the &mut self.

I did consider it. As I said, I spent hours on this and I am really open to a solution but I can't find one. What you say makes sense but what I understand of the issue is that Arc does not impl DerefMut and therefore there is no way I could possibly call those method on it (since they require a mutable reference).

If you want to give it a shot:

I need to change the trait PolkadotClient here so it impl BlockImport and I can call import (or import_block()) from there.

The reason is that, for any non-idempotent method, &self is always strictly worse than &mut self. It means "anything can modify the state of the underlying object at any time", which makes the logic way more difficult to understand.

I think I understand your point... if it wasn't wrapped in an Arc everything would be fine.

[tomaka]

What you say makes sense but what I understand of the issue is that Arc does not impl DerefMut and therefore there is no way I could possibly call those method on it (since they require a mutable reference).

You would pass a &mut Arc<Client>, not a &mut Client. It's Arc<Client> that would implement the trait.

I need to change the trait PolkadotClient here so it impl BlockImport and I can call import (or import_block()) from there.

Maybe re-define the import method on a new, different trait?
There's no reason why we have to change existing traits to account for new use cases just because they share a name.

[rphmeier]

BlockImport is supposed to have synchronous & unique access to the underlying database. There is absolutely no reason that it should be shared or we should be importing blocks in parallel. It would be much better to have a single BlockImport that components communicate with by message-passing.

[andresilva]

Overall this is what's happening:
https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=eda01c3939063c821f5c44f4d12b3f5f

As you can see on the first example in main it is able to call import even though we're using an Arc (https://github.com/paritytech/substrate/blob/master/test-utils/client/src/client_ext.rs#L86). The problem in your example is that you're not using Client from substrate but rather a trait PolkadotClient which introduces another layer of indirection. For some reason the compiler isn't able to prove that &PolkadotClient implements BlockImport which is needed for the BlockImportExt impl. Not sure how to fix it but testing on the playground above is probably easier.

[cecton]

Closing this PR for now but

[cecton]

Thanks to everybody for having tried to help me find a solution to this issue.

Big update: I won't re-open this ticket at all because importing a block was not what I needed to do for my task. 🙃 Mistakes were made!