Unstake and liquidate restrictions #1663
Conversation
| @@ -2561,7 +2561,7 @@ void core_rpc_server::invoke(BLS_REWARDS_REQUEST& bls_rewards_request, rpc_conte | |||
| bls_rewards_request.response["amount"] = bls_withdrawal_signature_response.amount; | |||
| bls_rewards_request.response["signed_message"] = bls_withdrawal_signature_response.signed_message; | |||
| bls_rewards_request.response["signature"] = bls_withdrawal_signature_response.signature; | |||
| bls_rewards_request.response["signers_bls_pubkeys"] = bls_withdrawal_signature_response.signers_bls_pubkeys; | |||
| bls_rewards_request.response["non_signers_bls_pubkeys"] = m_core.get_blockchain_storage().m_l2_tracker->get_non_signers(bls_withdrawal_signature_response.signers_bls_pubkeys); | |||
There was a problem hiding this comment.
Forewarning that this will crash on non-service nodes. We don't need to fix because we have a ticket item for it already but flagging it as a reminder.
|
|
||
| std::vector<uint64_t> RewardsContract::getNonSigners(const std::vector<std::string>& bls_public_keys) { | ||
| const uint64_t service_node_sentinel_id = 0; | ||
| ContractServiceNode service_node_end = serviceNodes(service_node_sentinel_id); |
There was a problem hiding this comment.
This function needs to take in a block height because there are race conditions with querying the service nodes with the default parameter of "latest"
There was a problem hiding this comment.
Interesting point! This seems like part of a bigger problem because the non signers list needs to be in sync with the smart contracts latest data for the transaction to succeed. If the list changes while we are traversing then we will need to throw away the old data, keeping the height the same will result in an invalid tx when you go to submit.
To go even further if you dont immediately submit this transaction after getting this data, and the smart contract is updated then the non signers data becomes invalid. The BLS signature might still be correct and valid, but the non signers list must always reflect the difference between who signed and the latest smart contract list.
However, the smart contract list changes slowly, so the likelihood of it changing during this process is relatively low. Therefore, it might not be an immediate concern. If a user encounters this problem, its probably reasonable to simply ask them to run the query again.
There was a problem hiding this comment.
Yeah that's a nasty problem. We can make the function call atomic (e.g. force the EVM to return the array of all the service nodes) in one shot so atleast the data is consistent.
But we probably also need additional checks that's like just as we compute it, we check if the contract hash has changed. If it has allow the operation to be retried 2-3 times to produce a command here the contract hash has remained the same
Then output that result to the user if it's like a BLS aggregation request or error and say that the contract was modified since requesting the call and the user has to try again.
I'd agree and say it's unlikely, and it can be held off as a nice to have if we have time. At the least though when we revisit the front-end or the BLS daemon RPC commands we can log a notice to the user that there's a possibility that the command may be invalidated inbetween the time it was generated and submitted because of this possibility just as a heads up on why someone might need to repeat the step.
jagerman
force-pushed
the
integration
branch
3 times, most recently
from
8aa4d63
to
a806b24
Compare
|
This LGTM with the review commit addressing the const patch. Everything else is optional nice-to-have's and can be addressed after. |
darcys22
force-pushed
the
unstake-and-liquidate-restrictions
branch
from
7db41db
to
42941e2
Compare
darcys22
force-pushed
the
unstake-and-liquidate-restrictions
branch
from
42941e2
to
1be7a11
Compare
No description provided.