Parsec atomic commit fix #246
Conversation
|
Thanks to @ayeshaali and @madars for identifying this |
|
This PR also contains a new util called |
maurermi
force-pushed
the
atomic_commit
branch
from
86ad55a to
e53cdd2
Compare
src/parsec/util.cpp
Outdated
| @@ -183,6 +184,8 @@ namespace cbdc::parsec { | |||
| cfg.m_runner_type = runner_type::evm; | |||
| } else if(val == "lua") { | |||
| cfg.m_runner_type = runner_type::lua; | |||
| } else if(val == "py") { | |||
There was a problem hiding this comment.
While I love this, I don't believe this should be in here - part of the other PR you are on AFAIK
There was a problem hiding this comment.
Good catch, thanks!
src/parsec/util.hpp
Outdated
| @@ -25,6 +25,8 @@ namespace cbdc::parsec { | |||
| lua, | |||
| /// Ethereum-style transactions using EVM. | |||
| evm, | |||
| /// Python Script using CPython | |||
| py, | |||
src/parsec/util.hpp
Outdated
| @@ -71,6 +73,20 @@ namespace cbdc::parsec { | |||
| broker::key_type key, | |||
| broker::value_type value, | |||
| const std::function<void(bool)>& result_callback) -> bool; | |||
|
|
|||
| /// Asynchronously get the value stored at key from the cluster. | |||
| /// Intended for testing purposes. | |||
There was a problem hiding this comment.
Nit-picky but potentially administrative functions as well like migrating keys? I wouldn't use that example but maybe the comment should say "testing and administration purposes" instead?
Yes, I did just add a review comment on a code comment... :)
There was a problem hiding this comment.
I can get on board with that, will update
There was a problem hiding this comment.
For context, this comment is trying to convey that this is not the optimal method for reading from the shards as a system component, it is better to handle shard responses in the agent
Adds a tool to get information directly from the shards, for debug and testing purposes. Signed-off-by: Michael Maurer <[email protected]>
Addresses a bug identified which allows for a smart contract to return an update map containing keys that the agent has not acquired tickets for. This fix ensures that when smart contract execution completes, all updates will be committed or none of them will. Previously, committed changes which the agent held write locks for would be committed, but changes which never had ticket requests made would silently fail. Note: if a key has a ticket but is waiting on a lock, the transaction rolls back and retries. If a ticket has not been acquired, the transaction should fail as the contract specifically needs to request locks on important keys. Signed-off-by: Michael Maurer <[email protected]>
Adds an integration test to ensure that updates either all commit or none of them do in parsec. Specifically ensures that a commit attempt where not all update keys have associated tickets aborts. Signed-off-by: Michael Maurer <[email protected]>
maurermi
force-pushed
the
atomic_commit
branch
from
e53cdd2 to
afc1b9c
Compare
|
utACK, clear and simple fix (core fix in 1e51249) to throw a reasonable error in the agent if a change-set includes modifications to keys which don't have tickets. I also want to call out how easy it is to review this PR because of the pleasant separation of the commits. CI passed, and the output for the added integration tests looks good: |
Fixes a bug which occurs when a smart contract does not request locks on all update keys, resulting in undesirable behavior.
Aborts a transaction where the agent does not have tickets associated with all update keys. In the case that a contract does not prompt for all update keys to be locked, enforces that the transaction aborts (+ rolls back) and no updates are written, rather than those which are locked. Does not abort if the smart contract correctly requested locks and is still waiting on them, but still rolls back and retries.