dispatch incoming messages #832

pro-wh · 2022-03-10T01:02:55Z

codecov · 2022-03-10T01:04:16Z

Codecov Report

Merging #832 (5c2f73c) into main (35b1adb) will decrease coverage by 39.99%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             main     #832       +/-   ##
===========================================
- Coverage   68.17%   28.18%   -40.00%     
===========================================
  Files         128       26      -102     
  Lines       11101     1902     -9199     
===========================================
- Hits         7568      536     -7032     
+ Misses       3501     1334     -2167     
  Partials       32       32

Impacted Files	Coverage Δ
runtime-sdk/modules/contracts/src/lib.rs
runtime-sdk/modules/evm/src/lib.rs
runtime-sdk/src/context.rs
runtime-sdk/src/dispatcher.rs
runtime-sdk/src/module.rs
runtime-sdk/src/modules/accounts/mod.rs
runtime-sdk/src/modules/consensus/mod.rs
runtime-sdk/src/modules/consensus_accounts/mod.rs
runtime-sdk/src/modules/core/mod.rs
runtime-sdk/src/modules/core/test.rs
... and 92 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

pro-wh · 2022-03-10T01:06:12Z

new systems, similar to what we have for transactions.

a prefetch step
runtime tries to parse the data into method+body structure, similar to Call, but no encryption support
- will probably need some kind of gas limit field
add a field to batch context to communicate out how many messages are processed

pro-wh · 2022-03-10T22:30:09Z

An idea came up in our internal discussion: maybe we could re-use a lot of existing systems by having the payload be (contain, imo) a transaction in the format we already have. I'm going to try coding that up.

pro-wh · 2022-03-12T01:20:39Z

turns out a lot of handler methods I previously declared as Result<_, core::Error> should really be using RuntimeError as the error type

kostko · 2022-03-15T12:37:37Z

runtime-sdk/src/dispatcher.rs

    ) -> Result<ExecuteBatchResult, RuntimeError> {
        let cfg = R::SCHEDULE_CONTROL.unwrap(); // Must succeed otherwise we wouldn't be here.
        let mut tx_reject_hashes = Vec::new();

        let mut result = self.execute_batch_common(
            rt_ctx,
            |ctx| -> Result<Vec<ExecuteTxResult>, RuntimeError> {
+                // Execute incoming messages.
+                for in_msg in in_msgs {


There should be a certain per-block gas limit reserved for incoming messages and we should only include so many. Additionally we could also include a per-msg gas limit.

added a per-block gas limit

pro-wh · 2022-03-17T00:02:42Z

added doc comments.

fixed mint_into_fee_accumulator not adjusting total supply

kostko

Hm so one thing I noticed is that this makes it quite hard to see what was actually processed and what was the result as all results are just dropped. Should we figure out a better way for handling these?

A simple way would be to emit events that contain the message ID and result of processing the message.

kostko · 2022-03-21T11:41:18Z

runtime-sdk/src/types/address.rs

@@ -250,6 +250,12 @@ impl From<Address> for ConsensusAddress {
    }
 }

+impl From<&ConsensusAddress> for Address {
+    fn from(addr: &ConsensusAddress) -> Address {
+        Address::from_bytes(addr.as_ref()).unwrap()


Let's add a unit test.

kostko · 2022-03-21T11:52:14Z

runtime-sdk/src/types/in_msg.rs

+    pub version: u16,
+    /// An embedded transaction (UnverifiedTransaction).
+    /// The transaction doesn't need to be from the same account that sent the message.
+    pub ut: Option<Vec<u8>>,


I see, so transactions are optional and you can also have messages that don't include transactions. I assume that if you want to just do a deposit, you don't need a transaction?

kostko · 2022-03-21T11:57:11Z

runtime-sdk/src/dispatcher.rs

+                        warn!(ctx.get_logger("dispatcher"), "incoming message data malformed"; "id" => in_msg.id, "err" => ?err);
+                        IncomingMessageData::noop()
+                    });
+                    let tx = match data.ut.as_ref() {


Hm so this implementation is not synced with just execute which may cause discrepancies (e.g. the proposer runs schedule_and_execute while workers only run execute). Should probably have a unified implementation if possible (could this go to execute_batch_common instead?).

it should be synced up, other than the logic to determine how many messages fit within the incoming messages gas limit. unification runs into the same concerns as the transaction processing: during non-scheduling execution, we've thus far preferred to parse all txes first; during scheduling execution, we parse and execute one by one.

oh although one thing about this: enforcing the incoming messages gas limit is not done under consensus. only the scheduler chooses

other than the logic to determine how many messages fit within the incoming messages gas limit

But the proposer does not communicate how many messages were chosen to executors. So if executors don't use the same logic for choosing messages, there will be discrepancies.

This would be different if we go with the design that emits incoming messages as transactions as then the proposer would decide how many to process (and executors would just need to verify that the messages are actually correct and in order as determined by the consensus layer).

oh okay then

kostko · 2022-03-21T11:58:45Z

runtime-sdk/src/dispatcher.rs

+        data: &IncomingMessageData,
+        tx: &Option<Transaction>,
+    ) -> Result<(), RuntimeError> {
+        R::Modules::execute_in_msg(ctx, in_msg, data, tx)?;


Note that returning an error here will cause a batch abort.

kostko · 2022-03-21T12:05:09Z

runtime-sdk/src/module.rs

+    }
+
+    /// Execute an incoming message, except for the embedded transaction. The dispatcher will
+    /// invoke the transaction and method handlers for the embedded transaction separately.


We should probably explicitly mention that returning an error here will cause a batch abort (and possibly stall the whole runtime as any future batch will also include this same message). So returning errors here must be considered carefully.

Also it should be noted that this executes in the batch-wide context and so any effects will not be rolled back even if the inner transaction fails.

pro-wh · 2022-03-21T18:53:39Z

discussed this morning: we'd like to take some more time to think about how to make the transactions delivered this way and the results of those transactions more easily available

pro-wh · 2022-03-26T00:11:26Z

another issue: we discard the transaction results, which includes the tags. that's too weird. I'm converting this PR to a draft.

pro-wh · 2022-04-26T23:51:16Z

cross reference oasisprotocol/oasis-core#4681

pro-wh · 2022-08-08T23:51:25Z

rebasing, but we'll need to redo major parts of this now that we'll be relying on oasis-core to replicate the incoming messages' data as transactions

pro-wh force-pushed the pro-wh/feature/inmsgs branch 3 times, most recently from b913b1c to 5c16442 Compare March 12, 2022 01:10

pro-wh force-pushed the pro-wh/feature/inmsgs branch from 5c16442 to b5b3ba5 Compare March 15, 2022 00:33

kostko reviewed Mar 15, 2022

View reviewed changes

pro-wh force-pushed the pro-wh/feature/inmsgs branch 4 times, most recently from 117ded0 to 524cfcc Compare March 15, 2022 23:44

pro-wh marked this pull request as ready for review March 16, 2022 23:18

pro-wh requested a review from ptrus as a code owner March 16, 2022 23:18

pro-wh force-pushed the pro-wh/feature/inmsgs branch from 52e2e08 to 2877907 Compare March 17, 2022 00:02

pro-wh force-pushed the pro-wh/feature/inmsgs branch from 2877907 to f83c2a8 Compare March 18, 2022 22:46

kostko reviewed Mar 21, 2022

View reviewed changes

pro-wh force-pushed the pro-wh/feature/inmsgs branch 2 times, most recently from 43b19fb to b9fee1e Compare March 26, 2022 00:01

pro-wh marked this pull request as draft March 26, 2022 00:10

pro-wh force-pushed the pro-wh/feature/inmsgs branch from b9fee1e to c85b000 Compare April 1, 2022 19:17

pro-wh force-pushed the pro-wh/feature/inmsgs branch from c85b000 to 06b2df5 Compare April 26, 2022 23:48

pro-wh mentioned this pull request Apr 26, 2022

go/runtime/txpool: transactions in incoming messages oasisprotocol/oasis-core#4681

Merged

pro-wh added 2 commits August 8, 2022 16:47

dispatcher: decode incoming messages

a174445

module: add incoming message handler

1fb5a8a

pro-wh added 8 commits August 8, 2022 16:47

modules/accounts: mint_into_fee_accumulator

41fd21b

modules/consensus_accounts: handle incoming message tokens

d39c92c

modules/core: add incoming messages gas limit parameter

1c718c3

dispatcher: watch incoming messages gas

01ce63c

dispatcher: add note about incoming messages events

57624df

f gas

6ea7b51

(unfinished) as part of txs

5688603

(junk) dedup

5c2f73c

pro-wh force-pushed the pro-wh/feature/inmsgs branch from 06b2df5 to 5c2f73c Compare August 8, 2022 23:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dispatch incoming messages #832

dispatch incoming messages #832

pro-wh commented Mar 10, 2022

codecov bot commented Mar 10, 2022 •

edited

Loading

pro-wh commented Mar 10, 2022

pro-wh commented Mar 10, 2022

pro-wh commented Mar 12, 2022

kostko Mar 15, 2022

pro-wh Mar 16, 2022

pro-wh commented Mar 17, 2022

kostko left a comment

kostko Mar 21, 2022

kostko Mar 21, 2022

kostko Mar 21, 2022

pro-wh Mar 21, 2022

pro-wh Mar 21, 2022

kostko Mar 22, 2022

pro-wh Mar 22, 2022

kostko Mar 21, 2022

kostko Mar 21, 2022

pro-wh commented Mar 21, 2022

pro-wh commented Mar 26, 2022

pro-wh commented Apr 26, 2022

pro-wh commented Aug 8, 2022

dispatch incoming messages #832

Are you sure you want to change the base?

dispatch incoming messages #832

Conversation

pro-wh commented Mar 10, 2022

codecov bot commented Mar 10, 2022 • edited Loading

Codecov Report

pro-wh commented Mar 10, 2022

pro-wh commented Mar 10, 2022

pro-wh commented Mar 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pro-wh commented Mar 17, 2022

kostko left a 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

pro-wh commented Mar 21, 2022

pro-wh commented Mar 26, 2022

pro-wh commented Apr 26, 2022

pro-wh commented Aug 8, 2022

codecov bot commented Mar 10, 2022 •

edited

Loading