Skip to content
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

Merged
merged 13 commits into from
Feb 6, 2025

Conversation

kevindeforth
Copy link
Contributor

@kevindeforth kevindeforth commented Feb 1, 2025

Resolves #131 :

  • add contract binaries to compiled-contracts
  • add pytest fixtures for contract compilation
  • improve pytests for contract update
  • add a pytest for state cleanup
  • changes the gas requirements for sign call from 50 teragas to 15

Benchmarks:
For 40 signatures:

Contract avg. receipts avg. gas [Tgas]
V0 8 11.30479597562405
V1 4 6.131075775468398

Contract Logic changes:
Each row is a promise, ignore empty rows.

V0 V1
do some checks do some checks, add request to state, schedule yield promise
add request to state, schedule yield promise
remove signature request from state clean state & return signature or error
return signature or error

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: Vector<(u64, SignatureRequest)>,
    config: ConfigV1,

request_by_timestamp is iterable and stores the env::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 to sign(), respond() or remove_timed_out_requests().

Note:
The signature of the sign function changes with this PR, specifically, the return value is no longer Result<near_sdk::Promise, Error>, but all Errors result in panic.

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.

@kevindeforth kevindeforth marked this pull request as ready for review February 1, 2025 17:20
Copy link
Contributor

@bowenwang1996 bowenwang1996 left a 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

@kevindeforth kevindeforth marked this pull request as draft February 3, 2025 13:28
@kevindeforth kevindeforth force-pushed the kd/optimize-chain-signatures-contract-for-latency branch 3 times, most recently from d0d7681 to 8c9493a Compare February 4, 2025 22:05
@kevindeforth kevindeforth marked this pull request as ready for review February 4, 2025 22:39
libs/chain-signatures/contract/src/config/impls.rs Outdated Show resolved Hide resolved
libs/chain-signatures/contract/src/lib.rs Outdated Show resolved Hide resolved
libs/chain-signatures/contract/src/config/impls.rs Outdated Show resolved Hide resolved
libs/chain-signatures/contract/src/lib.rs Outdated Show resolved Hide resolved
libs/chain-signatures/contract/src/lib.rs Outdated Show resolved Hide resolved
match self {
Self::V0(_) => {}
Self::V1(mpc_contract) => {
mpc_contract.remove_timed_out_requests();
Copy link
Contributor

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
@kevindeforth kevindeforth force-pushed the kd/optimize-chain-signatures-contract-for-latency branch from d159923 to b376b6c Compare February 5, 2025 19:34
adjust attached gas for pytest signature requests
libs/chain-signatures/contract/src/lib.rs Outdated Show resolved Hide resolved
libs/chain-signatures/contract/src/lib.rs Outdated Show resolved Hide resolved
pytest/common_lib/constants.py Outdated Show resolved Hide resolved
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.
Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor Author

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

libs/chain-signatures/contract/src/lib.rs Show resolved Hide resolved
Copy link
Contributor

@saketh-are saketh-are left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @kevindeforth!

@kevindeforth kevindeforth merged commit 71f8dfd into main Feb 6, 2025
1 check passed
@kevindeforth kevindeforth deleted the kd/optimize-chain-signatures-contract-for-latency branch February 6, 2025 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Optimize chain signatures contract for latency
3 participants