Implement #1641 rpc call public functions as read-only - take 2 #2527

psq · 2021-03-22T07:36:54Z

second attempt in fixing #1641 to allow calling any function as read-only. the call will fail if any write happened during the call.

For example, this allows calling public functions that call use contract-call?, which can not be used in a read-only function, even if the called contract function itself is read-only.

jcnelson · 2021-03-29T15:11:33Z

Thanks for sending this @psq! We'll try to get this included in 2.0.12.

pavitthrap · 2021-03-31T23:01:25Z

src/net/rpc.rs

-                },
-            ),
+            Ok(Some(Err(e))) => match e {
+                Unchecked(CheckErrors::CostBalanceExceeded(actual_cost, _))


I just want to clarify the "path" you're expecting execute_contract to follow to get to this branch of the match statement (where we get the CostBalanceExceeded error).

To me, it seems you want execute_contract to fail at its call to runtime_cost, which will check that the execution costs do not exceed those set by the limits the environment has set. However, the limit is equal to options.read_only_call_limit (as set in line 1250 of this file), and the value for this struct (type ExecutionCost) ultimately comes from ConnectionOptions, which is specified in the Config passed to initialize a new node. So, it seems like there is not a guarantee that write_count or write_length would be 0, meaning the call to runtime_cost in execute_contract may be okay even if there is a write.

I also had a question about this. I could actually see this going either way:

This API endpoint explicitly forbids any writes, and causes a CostBalanceExceeded error the moment the code attempts to wrote.

This API endpoint explicitly permits writes, but never materializes them. This could be used to allow clients to simulate what would happen if they called a function that performed a write, even though the write never commits.

I can see value in both approaches.

as described in #1641

Make it so that the RPC endpoint used for calling read-only functions can be used to call any functions. Make it so that any mutations are discarded

this makes it possible to call any function, and get a result if no write occurred. Furthermore, a read-only function that makes a contrac-call? is flagged as non read only, even if it is calling other read-only functions and the call-read endpoint will fail. So this only allow calling functions that are so basic it is not so useful in practice, and it prevents composing contracts. You have to use define-public instead, but this is not callable via the call-read endpoint.

So this extends the call-read endpoint to try the function, and return a result if no writes occurred, or a failure if any writes did occur.

So, @jcnelson, this is definitely the former.

@pavitthrap I'm not sure I understand how the node configuration would come into the picture. Either write_count is 0 after the execution and that's a read-only call, or it will fail because it was not read-only.

I think @pavitthrap's concern (correct me if I'm wrong) is that options.read_only_call_limit may have a non-zero values, since this ExecutionCost instance is instantiated from the node's config file, and you're allowed to specify non-zero write sizes. I think the intention for even permitting this at all is that one day, there would be an interface that lets you call read/write contract-calls such that the writes never materialize (or even hit disk).

I think the solution here would be to constract an ExecutionCost from options.read_only_call_limit and explicitly set the write_length and write_count fields to 0.

oh, @pavitthrap comment makes sense now. I wasn't expecting these values to be other that 0, but to be safe, I'll set them to 0.

addressed in 57b29c8

jcnelson · 2021-04-01T03:56:04Z

src/net/inv.rs

@@ -1456,7 +1456,7 @@ impl PeerNetwork {
        };

        debug!(
-            "{:?}: Send GetPoxInv to {:?} for {} rewad cycles starting at {} ({})",
+            "{:?}: Send GetPoxInv to {:?} for {} reward cycles starting at {} ({})",


Lol wee kant spel

pavitthrap · 2021-04-14T20:05:17Z

testnet/stacks-node/src/tests/integrations.rs

+    (ok (var-set x val)))
+";
+
+const MAIN_CONTRACT: &'static str = "


Could you change the variables here to be more descriptive of what we are trying to test? (ex: READ_ONLY_MAIN_CONTRACT)

addressed in 1e2ae7a

testnet/stacks-node/src/tests/integrations.rs

pavitthrap · 2021-04-14T20:11:26Z

src/net/rpc.rs

                    })
                    .map_err(|_| {
                        ClarityRuntimeError::from(InterpreterError::CostContractLoadFailure)
                    })?;

                clarity_tx.with_readonly_clarity_env(mainnet, sender.clone(), cost_track, |env| {
-                    env.execute_contract(&contract_identifier, function.as_str(), &args, true)
+                    env.execute_contract(&contract_identifier, function.as_str(), &args, false)


Could you add a comment here explaining why we are setting this to false here (something about how we are using the limits imposed by the cost tracker, which is owned by the environment, to detect any writes).

sure, will do.

addressed in 1e2ae7a

jcnelson

LGTM! Thanks @psq!

jcnelson · 2021-04-19T16:09:35Z

Just a reminder: please don't merge until 2.0.11 is released.

psq · 2021-04-20T04:43:48Z

thank you both for the review and the back and forth, which made this PR better than what it started as.

philiphacks · 2021-04-22T15:19:17Z

thanks for the PR @psq

would be great if this can land in 2.0.12 as this blocks arkadiko from dynamically versioning contracts 👀

psq · 2021-04-23T00:13:44Z

thanks for the PR @psq

would be great if this can land in 2.0.12 as this blocks arkadiko from dynamically versioning contracts 👀

swapr also relies heavily on this, but would run my own node with this PR deployed if necessary to extract data from contract, but let Connect send transactions through the regular API node (but would rather not have to do this, of course).

Not available yet on mainnet, just on my own mocknet. Happy to share if 2.0.12 gets delayed for any reason.

kantai

@psq -- these changes all look good to me. Can you resolve the conflicts in integrations and make sure that the test passes?

psq · 2021-04-27T00:17:11Z

@kantai sure, will rebase

psq · 2021-04-27T02:35:20Z

@kantai rebased and all green

blockstack-devops · 2024-11-22T00:23:40Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

psq requested review from jcnelson and kantai March 22, 2021 07:36

psq changed the title ~~Implement #1641 rpc call public functions as read-only~~ Implement #1641 rpc call public functions as read-only - take 2 Mar 22, 2021

psq self-assigned this Mar 22, 2021

pavitthrap reviewed Mar 31, 2021

View reviewed changes

jcnelson reviewed Apr 1, 2021

View reviewed changes

jcnelson mentioned this pull request Apr 2, 2021

More fine-grained allow/deny modes for post-conditions #2557

Open

psq requested review from jcnelson and pavitthrap April 13, 2021 20:36

pavitthrap reviewed Apr 14, 2021

View reviewed changes

psq force-pushed the feat/rpc-call-read-1641-v2 branch from 57b29c8 to 1e2ae7a Compare April 15, 2021 21:56

pavitthrap approved these changes Apr 16, 2021

View reviewed changes

jcnelson approved these changes Apr 19, 2021

View reviewed changes

psq mentioned this pull request Apr 21, 2021

feat: add traits for critical smart contracts to allow dynamic versioning of contracts philiphacks/arkadiko-dao#62

Merged

kantai reviewed Apr 26, 2021

View reviewed changes

psq added 6 commits April 26, 2021 18:00

try readonly and fail if it wrote instead

80d246d

chore: rustfmt

be29cd8

make sure write_count and write_length are set to 0

d871d65

address review feedback

215819b

3 characters too long

68bc4a6

rebase and fix nonces

74d3595

psq force-pushed the feat/rpc-call-read-1641-v2 branch from 17a889b to 74d3595 Compare April 27, 2021 01:08

jcnelson merged commit f6697f3 into stacks-network:develop Apr 27, 2021

blockstack-devops added the locked label Nov 22, 2024

stacks-network locked as resolved and limited conversation to collaborators Nov 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement #1641 rpc call public functions as read-only - take 2 #2527

Implement #1641 rpc call public functions as read-only - take 2 #2527

psq commented Mar 22, 2021

jcnelson commented Mar 29, 2021

pavitthrap Mar 31, 2021

jcnelson Apr 1, 2021

psq Apr 1, 2021

jcnelson Apr 1, 2021

psq Apr 1, 2021

psq Apr 1, 2021

jcnelson Apr 1, 2021

pavitthrap Apr 14, 2021

psq Apr 14, 2021

psq Apr 15, 2021

pavitthrap Apr 14, 2021

psq Apr 14, 2021

psq Apr 15, 2021

jcnelson left a comment

jcnelson commented Apr 19, 2021

psq commented Apr 20, 2021

philiphacks commented Apr 22, 2021

psq commented Apr 23, 2021

kantai left a comment

psq commented Apr 27, 2021

psq commented Apr 27, 2021

blockstack-devops commented Nov 22, 2024

Implement #1641 rpc call public functions as read-only - take 2 #2527

Implement #1641 rpc call public functions as read-only - take 2 #2527

Conversation

psq commented Mar 22, 2021

jcnelson commented Mar 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcnelson left a comment

Choose a reason for hiding this comment

jcnelson commented Apr 19, 2021

psq commented Apr 20, 2021

philiphacks commented Apr 22, 2021

psq commented Apr 23, 2021

kantai left a comment

Choose a reason for hiding this comment

psq commented Apr 27, 2021

psq commented Apr 27, 2021

blockstack-devops commented Nov 22, 2024