dan-da
commented
3 months ago iiuc, you are proposing to change this line:
to:
let state_mut = self.state.lock_guard_mut().await; and hold the state_mut lock until after the call to create_transaction().
This means holding the lock across:
- generate_sender_randomness()
- generate_public_announcement()
- rpc_server_to_main_tx.send()
I'm unsure how expensive (or not) these fn are. Probably not very, though I do notice that generate_public_announcement() calls an encryption function GenerationAddress::encrypt().
The philosophy I have been operating under is to keep write-locks as short as possible, ie no longer than necessary in order to preserve atomicity. This facilitates concurrency.
In this case, I doubt it will make a big difference, so I'm not totally averse to the change. It just seems to sort of go against that philosophy and I don't see a need for it presently.
In its current form, the data that is read while the read-lock is held is not part of consensus, so the transactions should still be valid
exactly.
But the code in its current form is a pitfall for future rewrites, if more data is acquired while the read-lock is held.
I would prefer just to add a comment above the read-lock acquisition warning that no consensus data should be read, else write lock must be used instead.
Would that satisfy the concern?
If not, I will use a write-lock as you request.
Sword-Smith
Currently the transaction-generating endpoint in the RPC server takes a read-lock, releases it, and then takes a write-lock on
GlobalState. It should take a write-lock (since we eventually want to update the wallet state that some monitored UTXOs were the input to a transaction) that is held over the entire duration of the transaction generating process.In its current form, the data that is read while the read-lock is held is not part of consensus, so the transactions should still be valid. But the code in its current form is a pitfall for future rewrites, if more data is acquired while the read-lock is held.