Lock mutex in more client methods.

[afck]

Motivation

The tests in #2538 fail. A (possibly the) scenario that can cause a processInbox mutation to unexpectedly not produce a block is the following:

• Chain 1 creates a block B that sends a message to chain 2.
• The running node service of the owner of chain 2 receives the notification from the validators.
• The node service's client listener downloads B and starts processing it. It updates the chain state and puts the message in the outbox…
• Now processInbox is called: It sees that B has already been handled. On the other hand, the inbox is still empty, so it doesn't create a block.
• …finally the client listener task puts the messages in chain 2's inbox.

Proposal

Use the mutex in the ChainState in more places, to ensure that certain tasks don't overlap.

Test Plan

I ran the test_wasm_end_to_end_fungible::storage_service_grpc test locally 50 times successfully, together with the optimization in #2538 and the fix in #2562.

Release Plan

• These changes should be backported to the latest devnet branch, then
  • be released in a new SDK.
• These changes should be backported to the latest testnet branch, then
  • be released in a new SDK.

Links

• reviewer checklist

[ma2bd]

Why is this pub ?

[afck]

Because the ChainClient has to use it. I can make it pub(super).

[Twey]

This whole struct is private to linera_core::client. I'm usually suspicious of pub(super), pub(crate) and friends because, even though they're sometimes necessary (especially for use in macros), they imply non-local knowledge of the structure the code is embedded into. The upshot is that types look different (expose different behaviour) depending on where they're imported, which usually just results in a lot of spurious changes when refactoring, but coupled with conditional compilation could lead to some hard-to-spot compilation breakages.

[afck]

Yes, I also prefer just using pub in these cases. Happy to revert this in a later PR.

[ma2bd]

As a rule, public APIs need to be minimized.

[afck]

I'd argue that they are, since ChainState itself is only visible in the client module, and not exported further. So effectively all it's methods are pub(super) anyway.

[ma2bd]

Do we need two separate lines when lock_owned is used? (and otherwise why lock_owned?)

[afck]

We actually do! self.state() returns a ChainGuard that can't be held across an await point… specifically so we wouldn't ever lock an entry in the chain states map locked.

And it looks like Rust would drop that guard only after the whole expression, i.e. after the await.

[Twey]

This mutex is a hack, but I'm happy enough with the hack being extended to cover more cases in anticipation of a larger linera_core::client refactor. I think it's good that at least the locking logic remains within the client this time.