-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Reduce mpc contract latency #168
Reduce mpc contract latency #168
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Meta comment: given that V1 is not live yet, let's not introduce V2 unnecessarily and collapse changes in V2 into V1
d0d7681
to
8c9493a
Compare
match self { | ||
Self::V0(_) => {} | ||
Self::V1(mpc_contract) => { | ||
mpc_contract.remove_timed_out_requests(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be removed now that sign
takes the responsibility for cleanup?
add contract binaries to compiled-contracts add pytest fixtures for contract compilation add test contract update test to pytest
handle signature requests from v1 gracefully rebase conflicts add test for handling of trailing signature requests during update call impose limit on max number of requests removed
address pr comments
d159923
to
b376b6c
Compare
adjust attached gas for pytest signature requests
Co-authored-by: Bowen Wang <[email protected]>
Co-authored-by: Bowen Wang <[email protected]>
cluster.send_and_await_signature_requests(2, add_gas=150 * TGAS) | ||
|
||
|
||
# In case a nonce conflict occurs during a vote_update call, rerun the test once. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my previous testing I was not able to produce nonce conflicts in localnet even under significant load (10s or 100s of txs). I was running the other pytest test_signature_request with high num_requests and didn't see any requests fail to be included. I believe this is because the local setup is too simple for transaction reordering to happen. So I wonder if there is some other underlying cause for flakiness observed here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the timespan between the rpc request to fetch the current nonce value and until the update transaction is sent, the mpc participant already submitted other transactions and the nonce increased.
This improved significantly after using release version of near core, but I still get the error every now an then.
Still learning about keys and accounts. Does each access key have its own nonce and would it help to use a different access key for the update calls?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I opened an issue for this, as this PR is already quite large: #182
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @kevindeforth!
Resolves #131 :
Benchmarks:
For 40 signatures:
Contract Logic changes:
Each row is a promise, ignore empty rows.
Additional Contract State:
Because of the new logic, failed signature requests need to be periodically removed from the state.
This requires additional contract state:
request_by_timestamp
is iterable and stores theenv::block_height()
during which the request was added to the state.config
contains is the timeout after which signature requests can be removed from the contract, it also defines the maximum number of requests that can be removed during a call tosign()
,respond()
orremove_timed_out_requests()
.Note:
The signature of the
sign
function changes with this PR, specifically, the return value is no longerResult<near_sdk::Promise, Error>
, but all Errors result inpanic
.edit:
CI time:
The new pytest takes a long time and increases the ci time to 40 minutes, which seems quite long. This is expected to increase as we add more tests. If reviewers agree, I would like to restrict the ci to
pytest -m "not slow"
, skipping all tests that are marked as slow via@pytest.mark.slow
Alternatively, we could add a marker
ci
to annotate tests that should be included in ci, ignoring all tests that do not have that marker.