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

Add RPC endpoint to call tx without commit #4514

Merged
merged 1 commit into from
Sep 13, 2022

Conversation

gegaowp
Copy link
Contributor

@gegaowp gegaowp commented Sep 7, 2022

Tests:

  1. curl with prepared arguments like tx bytes etc.
  2. test with wallet transfer & NFT mint, making sure tx can still be executed
curl --location --request POST http://127.0.0.1:9000 \
--header 'Content-Type: application/json' \
--data-raw '{ "jsonrpc":"2.0", "method":"sui_callTransaction","id":1, "params":["VHJhbnNhY3Rpb25EYXRhOjoAA6cX+ums6YaOYWcwGNuac9DsItb+AWcrAAAAAAAA81CQ+9jZWfQZAMaenws7ICwj3rYjuxrAqWGpFXg4HaW8XtjMjfnHFAEAAAAAAAAAIFvTUM2y6Wk8Dm6HoTaarmIl7ddopKmibbkOkzJtxbkiAQAAAAAAAABkAAAAAAAAAA==", "ED25519", "1us4z7FQoI9KxQNVlAh6ZxPniX56QEE9odgs+XmXfAwWv83aDR/ch4WXd+PGVnArnicAAombbaHAjMOhnsgUBw==", "0QMT8bP3W5YppT2aWb3HD7NPWMSxfOxKUuET+xlaNiQ="]}'
{"jsonrpc":"2.0","result":{"status":{"status":"success"},"gasUsed":{"computationCost":45,"storageCost":48,"storageRebate":32},"transactionDigest":"rm6LcSKTYJ5cXYnpwaYFwUUsfGGqij0kxkzjbOOk2DI=","created":[{"owner":{"AddressOwner":"0xa717fae9ace9868e61673018db9a73d0ec22d6fe"},"reference":{"objectId":"0xdaab3a82b9ddde937fe5a55a53e10212007b2257","version":1,"digest":"s/rNVIQoTTpL9DFSZI8c7ubOuFKzdgdx18lFQWBs80w="}}],"mutated":[{"owner":{"AddressOwner":"0xf35090fbd8d959f41900c69e9f0b3b202c23deb6"},"reference":{"objectId":"0x23bb1ac0a961a91578381da5bc5ed8cc8df9c714","version":2,"digest":"sa1Mb0xP9WOGXJFKppwNORmvxSEnZfLdci18uFYxlfQ="}}],"gasObject":{"owner":{"AddressOwner":"0xf35090fbd8d959f41900c69e9f0b3b202c23deb6"},"reference":{"objectId":"0x23bb1ac0a961a91578381da5bc5ed8cc8df9c714","version":2,"digest":"sa1Mb0xP9WOGXJFKppwNORmvxSEnZfLdci18uFYxlfQ="}},"events":[{"transferObject":{"packageId":"0x0000000000000000000000000000000000000002","transactionModule":"native","sender":"0xf35090fbd8d959f41900c69e9f0b3b202c23deb6","recipient":{"AddressOwner":"0xa717fae9ace9868e61673018db9a73d0ec22d6fe"},"objectId":"0x23bb1ac0a961a91578381da5bc5ed8cc8df9c714","version":1,"type":"Coin","amount":11111}}],"dependencies":["D5EwdUY7/XB2HbMAXSBVH81Wj+/uavhoYVlg9b3bMfg="]},"id":1}%

Comment on lines 120 to 128
#[method(name = "callTransaction")]
async fn call_transaction(
&self,
tx_bytes: Base64,
sig_scheme: SignatureScheme,
signature: Base64,
pub_key: Base64,
) -> RpcResult<SuiTransactionEffects>;

Copy link
Contributor

@patrickkuo patrickkuo Sep 8, 2022

Choose a reason for hiding this comment

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

just my personal opinion :)

will calling it dry_run_transaction makes it more obvious that this method doesn't commit? call_transaction is a bit ambiguous for me.

Copy link
Contributor

Choose a reason for hiding this comment

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

also I think it makes more sense to put it in QuorumDriverApi together with execute_transaction

Copy link
Contributor Author

@gegaowp gegaowp Sep 8, 2022

Choose a reason for hiding this comment

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

@patrickkuo dry_run_transaction is a better name indeed, will change it accordingly!

to put it in QuorumDriverApi

I was thinking about this as well, if I do it this way, iirc the "dry_tx" will go to NetworkAuthorityClient and then try to reach validators, instead of executing it on the local fullnode following the current impl.

Executing locally on the fullnode is better I think bc

  • its not necessary to involve validators here
  • dry run request traffic on the fullnode is isolated from other nodes in case of abusing

Did I miss sth?

Copy link
Contributor

Choose a reason for hiding this comment

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

to do this we will need to pass in the AuthorityState to the FullNodeQuorumDriverApi, this should execute locally.

Having 2nd thoughts, the QuorumDriver is configurable, so fullnode can choose to disable it, maybe it's not a good idea to put it in QuorumDriverApi depends on the use case.

Copy link
Contributor Author

@gegaowp gegaowp Sep 8, 2022

Choose a reason for hiding this comment

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

the QuorumDriver is configurable, so fullnode can choose to disable it

right, iirc QuorumDriver drives txns with a quorum of validators as the name says, and dry_run to me is more like a read-only endpoint.


// At this point we need to check if any shared objects need locks,
// and whether they have them.
let shared_object_refs = input_objects.filter_shared_objects();
if !shared_object_refs.is_empty() && !certificate.signed_data.data.kind.is_system_tx() {
if !shared_object_refs.is_empty() && !transaction.signed_data.data.kind.is_system_tx() {
Copy link
Contributor

Choose a reason for hiding this comment

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

If the transaction contains shared objects, we need to ensure they have been scheduled
// for processing by the consensus protocol.

Does this also apply to call_transaction? If so, it seems to open up a vulnerability for DDOS where consensus protocol can be called without gas. cc @lxfind

Copy link
Contributor Author

Choose a reason for hiding this comment

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

iirc, check_shared_locks only checks local DB of AuthorityState, and the fullnode that processes this dry_run request will not send it elsewhere either, so it will not add extra traffic or workload to other nodes.
https://github.com/gegaowp/sui/blob/5d346fdcceb3e0f42b2b23feb7cffd367a98ed49/crates/sui-core/src/authority.rs#L530
will leave it to Xun to confirm.

Copy link
Contributor

Choose a reason for hiding this comment

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

Correct, this only checks for shared object locks, and does not do consensus.
However, if a transaction does involve shared objects, trying to execute it here without consensus is not going to work.
What do we want to do with call_transaction that involves shared objects? Maybe we should abort it earlier in call_transaction by checking this first.

Copy link
Contributor Author

@gegaowp gegaowp Sep 8, 2022

Choose a reason for hiding this comment

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

@lxfind thanks for the info!

trying to execute it here without consensus is not going to work.

other than the sequence number, does the execution also need anything else from consensus?

What do we want to do with call_transaction that involves shared objects?

the original idea is to implement an eth_call equivalent so ideally we can dry run txns with shared objects as well, but if that is indeed tricky, I think it would still be useful for owned objects only. And yes I should abort earlier if do so.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sequence number is what it needs, which is usually set by the consensus.
One alternative is that you could just use whatever is the latest state of the shared objects to execute the transaction, which may be useful in most cases if the shared object is not updated all the time. To do that, you can probably bypass the shared object lock check if it's attempting to execute a transaction instead of a certificate.
Some tests would be useful just to make sure this works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lxfind I made a couple of changes accordingly and it should be ready for another review, thanks!

  • dry run will skip the shared objects check, I also reverted the helper function change as code sharing is small now
  • added a dry run test.

@gegaowp gegaowp merged commit c55adde into MystenLabs:main Sep 13, 2022
@gegaowp gegaowp deleted the sui-call-rpc branch September 13, 2022 00:13
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.

None yet

4 participants