Paych actor: Drop account req, use AuthenticateMessage to verify sigs #824

arajasek · 2022-11-09T21:49:46Z

Part of #424.

In addition to obviating the need for Paych to/from addresses to be accounts, this also removes the need for them to exist in state at the time of creation -- this PR makes it possible to supply to/from ID addresses that don't actually exist in state at the time of paych creation.

We need to decide if this is something we want, or is undesirable. If the latter, we probably want a new syscall to check for ID addrs existing in state (we could achieve this by misusing existing syscalls, but I would prefer not to).

codecov-commenter · 2022-11-09T22:16:53Z

Codecov Report

❗ No coverage uploaded for pull request base (integration/builtin-api@54c278f). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head 2840559 differs from pull request most recent head 71194a8. Consider uploading reports for the commit 71194a8 to get more accurate results

Additional details and impacted files

@@                    Coverage Diff                     @@
##             integration/builtin-api     #824   +/-   ##
==========================================================
  Coverage                           ?   88.19%           
==========================================================
  Files                              ?       96           
  Lines                              ?    19884           
  Branches                           ?        0           
==========================================================
  Hits                               ?    17537           
  Misses                             ?     2347           
  Partials                           ?        0

anorth

this PR makes it possible to supply to/from ID addresses that don't actually exist in state at the time of paych creation.
We need to decide if this is something we want, or is undesirable.

I think it's undesirable. It's almost certainly an error from the caller, so a regression in functionality here.

anorth · 2022-11-11T00:16:25Z

actors/paych/src/ext.rs

+pub mod account {
+    use super::*;
+
+    pub const AUTHENTICATE_MESSAGE_METHOD: u64 = 3;


This should use the exported frc42 method number in order to support other actors.

Quite right, thank you.

actors/paych/src/lib.rs

anorth · 2022-11-11T01:16:02Z

actors/paych/src/lib.rs

+        // Resolve both parties if necessary.
+        // Note that this does NOT guarantee that the parties exist in state yet.
+        let to =
+            rt.resolve_address(&params.to).with_context_code(ExitCode::USR_NOT_FOUND, || {


The prior code returned USR_ILLEGAL_ARGUMENT here (in resolve_to_actor_id as well as if the actor type was wrong). The FRC46 token uses USR_NOT_FOUND.

In the first instance, I think it's better not to change behaviour right now, so use ILLEGAL_ARGUMENT. However, this address resolution thing strikes me as fundamental enough that maybe we should dedicate a new user exit code for it? Let's discuss in Slack.

actors/paych/tests/paych_actor_test.rs

anorth · 2022-11-11T01:44:48Z

actors/paych/tests/paych_actor_test.rs

+        rt.expect_send(
+            payer_addr,
+            AUTHENTICATE_MESSAGE_METHOD,
+            RawBytes::serialize(AuthenticateMessageParams {
+                signature: sv.clone().signature.unwrap().bytes,
+                message: sv.signing_bytes().unwrap(),
+            })
+            .unwrap(),
+            TokenAmount::zero(),
+            RawBytes::default(),
+            ExitCode::OK,
+        );


This block is duplicated enough to pull out to a helper function

anorth · 2022-11-11T01:46:03Z

FYI @Stebalien

anorth · 2022-11-15T22:46:31Z

actors/paych/src/lib.rs

+            // if raw was an ID address, we need to confirm that it actually exists in the state tree
+            Protocol::ID => {
+                let resolved = raw.id().map_err(|_| {
+                    actor_error!(illegal_state, "failed to convert ID address to ID")


Nit: Just unwrap() this, we do it everywhere. I think if it deserialized and has ID protocol, this should not fail.

anorth · 2022-11-15T22:59:04Z

actors/paych/src/lib.rs

+                        format!("no code for address {}", raw)
+                    })
+            }
+            // just resolve all other cases, will fail if not in state tree


I don't believe this. Knowing that the Init actor has one map of Address -> ID and another ID -> State, I would expect only the former to be inspected when resolving an address.

When we delete an actor, the address mapping remains but the actor state is deleted. Since the actor doesn't exist any more, I think we want this to fail to resolve in this use case (ref)

I get a sense you were trying to avoid get_actor_code_cid, but I really do think it's the perfect thing. The prior structure of (1) resolve to an ID address, then (2) check the actor exists (has code) was better.

Agreed, fixed

anorth · 2022-11-15T23:08:25Z

runtime/src/builtin/shared.rs

-/// ResolveToActorID resolves the given address to it's actor ID.
-/// If an actor ID for the given address dosen't exist yet, it tries to create one by sending
+/// ResolveToActorID resolves the given address to its actor ID.
+/// If an actor ID for the given address doesn't exist yet, it tries to create one by sending


…#824) * Paych actor: Drop account req, use AuthenticateMessage to verify sigs * Address review * Address review

* Remove the market actor state mutation pattern (#734) Fixes #656 * Proof of concept exported API for Account actor (#797) * Export stable methods for public access (#807) * Export Datacap Actor methods * Export Init Actor methods * Export Market Actor methods * Export Miner Actor methods * Export Multisig Actor methods * Export Verifreg Actor methods * Address review * Restrict internal APIs of all actors (#809) * Exported API method for market actor escrow/locked balance (#812) * Power actor: Add exported getters for raw power (#810) * Power actor: Add exported getters for raw power * FRC-XXXX is FRC-0042 * Power actor: network_raw_power: Return this_epoch_raw_byte_power * Power actor: miner_raw_power: Return whether above consensus min power * Power actor: types: serialize one-element structs transparently * Address review * Miner actor: Add exported getters for info and monies (#811) * Miner actor: Add exported getters for info and monies * Tweak comment * Miner actor: Replace GetWorker and GetControls with IsControllingAddress * Miner actor: Add exported GetAvailableBalance * Miner actor: Add exported GetVestingFunds * Miner actor: Remove exported monies getters * Miner actor: types: serialize one-element structs transparently * Address review * Address review * Built-in market API for deal proposal metadata (#818) * Call exported authenticate method from PSD (#829) Co-authored-by: zenground0 <[email protected]> * Drop CALLER_TYPES_SIGNABLE and signable caller validation (#821) * Market actor: Minor improvements to two exported getters (#826) * Market actor: GetDealTermExported: Return (start_epoch, duration) * Market actor: Export getter for deal total price * Exported API for market deal activation state (#819) * Paych actor: Drop account req, use AuthenticateMessage to verify sigs (#824) * Paych actor: Drop account req, use AuthenticateMessage to verify sigs * Address review * Address review * Account actor: Deprecate AuthenticateMessage (#856) * Market actor: Export PublishStorageDeals (#857) * Miner: Export several more methods (#863) * Miner: Export ChangeWorkerAddress * Miner: Export ChangePeerID * Miner: Export WithdrawBalance * Miner: Export ChangeMultiaddrs * Miner: Export ConfirmUpdateWorkerKey * Miner: Export RepayDebt * Miner: Export ChangeOwnerAddress * Miner: Add exported getters for PeerID & multiaddrs * Miner: Refactor: Rename ConfirmUpdateWorkerKey to ConfirmChangeWorkerAddress * Power actor: Export methods to CreateMiner and get miner counts (#868) * Power: Export CreateMiner * Power Actor: Export MinerCount and MinerConsensusCount * Update actors/power/src/lib.rs Co-authored-by: Alex <[email protected]> Co-authored-by: Alex <[email protected]> * Verifreg: Export AddVerifiedClient and GetClaims (#873) * Verifreg: Rename AddVerifierClientParams to AddVerifiedClientParams * Verifreg: Export AddVerifiedClient and GetClaims * Datacap actor: Modify exported methods (#909) * Datacap: Export Mint and Destroy * Datacap actor: Deprecate all internal methods * Datacap actor: Rename BalanceOf to Balance * Datacap actor: Add Granularity method * fix: comments on newly exported methods (#910) Co-authored-by: RK <[email protected]> Co-authored-by: Alex <[email protected]> Co-authored-by: ZenGround0 <[email protected]> Co-authored-by: zenground0 <[email protected]>

…#824) * Paych actor: Drop account req, use AuthenticateMessage to verify sigs * Address review * Address review

* Proof of concept exported API for Account actor (#797) * Export stable methods for public access (#807) * Export Datacap Actor methods * Export Init Actor methods * Export Market Actor methods * Export Miner Actor methods * Export Multisig Actor methods * Export Verifreg Actor methods * Address review * Restrict internal APIs of all actors (#809) * Exported API method for market actor escrow/locked balance (#812) * Power actor: Add exported getters for raw power (#810) * Power actor: Add exported getters for raw power * FRC-XXXX is FRC-0042 * Power actor: network_raw_power: Return this_epoch_raw_byte_power * Power actor: miner_raw_power: Return whether above consensus min power * Power actor: types: serialize one-element structs transparently * Address review * Miner actor: Add exported getters for info and monies (#811) * Miner actor: Add exported getters for info and monies * Tweak comment * Miner actor: Replace GetWorker and GetControls with IsControllingAddress * Miner actor: Add exported GetAvailableBalance * Miner actor: Add exported GetVestingFunds * Miner actor: Remove exported monies getters * Miner actor: types: serialize one-element structs transparently * Address review * Address review * Built-in market API for deal proposal metadata (#818) * Call exported authenticate method from PSD (#829) Co-authored-by: zenground0 <[email protected]> * Drop CALLER_TYPES_SIGNABLE and signable caller validation (#821) * Market actor: Minor improvements to two exported getters (#826) * Market actor: GetDealTermExported: Return (start_epoch, duration) * Market actor: Export getter for deal total price * Exported API for market deal activation state (#819) * Paych actor: Drop account req, use AuthenticateMessage to verify sigs (#824) * Paych actor: Drop account req, use AuthenticateMessage to verify sigs * Address review * Address review * Account actor: Deprecate AuthenticateMessage (#856) * Market actor: Export PublishStorageDeals (#857) * Miner: Export several more methods (#863) * Miner: Export ChangeWorkerAddress * Miner: Export ChangePeerID * Miner: Export WithdrawBalance * Miner: Export ChangeMultiaddrs * Miner: Export ConfirmUpdateWorkerKey * Miner: Export RepayDebt * Miner: Export ChangeOwnerAddress * Miner: Add exported getters for PeerID & multiaddrs * Miner: Refactor: Rename ConfirmUpdateWorkerKey to ConfirmChangeWorkerAddress * Power actor: Export methods to CreateMiner and get miner counts (#868) * Power: Export CreateMiner * Power Actor: Export MinerCount and MinerConsensusCount * Update actors/power/src/lib.rs Co-authored-by: Alex <[email protected]> Co-authored-by: Alex <[email protected]> * Verifreg: Export AddVerifiedClient and GetClaims (#873) * Verifreg: Rename AddVerifierClientParams to AddVerifiedClientParams * Verifreg: Export AddVerifiedClient and GetClaims * Datacap actor: Modify exported methods (#909) * Datacap: Export Mint and Destroy * Datacap actor: Deprecate all internal methods * Datacap actor: Rename BalanceOf to Balance * Datacap actor: Add Granularity method * fix: comments on newly exported methods (#910) * Miner: Export method to GetPendingOwner * MarketNotifyDeal (#944) Co-authored-by: zenground0 <[email protected]> * Verifreg: Call AuthenticateMessage instead of validating siggys * Multisig: Do not export any functionality (#967) * use const for EX_DEAL_EXPIRED (#954) * Address review Co-authored-by: Alex <[email protected]> Co-authored-by: ZenGround0 <[email protected]> Co-authored-by: zenground0 <[email protected]> Co-authored-by: Shashank <[email protected]>

…filecoin-project#824) * Paych actor: Drop account req, use AuthenticateMessage to verify sigs * Address review * Address review

…ecoin-project#1004) * Proof of concept exported API for Account actor (filecoin-project#797) * Export stable methods for public access (filecoin-project#807) * Export Datacap Actor methods * Export Init Actor methods * Export Market Actor methods * Export Miner Actor methods * Export Multisig Actor methods * Export Verifreg Actor methods * Address review * Restrict internal APIs of all actors (filecoin-project#809) * Exported API method for market actor escrow/locked balance (filecoin-project#812) * Power actor: Add exported getters for raw power (filecoin-project#810) * Power actor: Add exported getters for raw power * FRC-XXXX is FRC-0042 * Power actor: network_raw_power: Return this_epoch_raw_byte_power * Power actor: miner_raw_power: Return whether above consensus min power * Power actor: types: serialize one-element structs transparently * Address review * Miner actor: Add exported getters for info and monies (filecoin-project#811) * Miner actor: Add exported getters for info and monies * Tweak comment * Miner actor: Replace GetWorker and GetControls with IsControllingAddress * Miner actor: Add exported GetAvailableBalance * Miner actor: Add exported GetVestingFunds * Miner actor: Remove exported monies getters * Miner actor: types: serialize one-element structs transparently * Address review * Address review * Built-in market API for deal proposal metadata (filecoin-project#818) * Call exported authenticate method from PSD (filecoin-project#829) Co-authored-by: zenground0 <[email protected]> * Drop CALLER_TYPES_SIGNABLE and signable caller validation (filecoin-project#821) * Market actor: Minor improvements to two exported getters (filecoin-project#826) * Market actor: GetDealTermExported: Return (start_epoch, duration) * Market actor: Export getter for deal total price * Exported API for market deal activation state (filecoin-project#819) * Paych actor: Drop account req, use AuthenticateMessage to verify sigs (filecoin-project#824) * Paych actor: Drop account req, use AuthenticateMessage to verify sigs * Address review * Address review * Account actor: Deprecate AuthenticateMessage (filecoin-project#856) * Market actor: Export PublishStorageDeals (filecoin-project#857) * Miner: Export several more methods (filecoin-project#863) * Miner: Export ChangeWorkerAddress * Miner: Export ChangePeerID * Miner: Export WithdrawBalance * Miner: Export ChangeMultiaddrs * Miner: Export ConfirmUpdateWorkerKey * Miner: Export RepayDebt * Miner: Export ChangeOwnerAddress * Miner: Add exported getters for PeerID & multiaddrs * Miner: Refactor: Rename ConfirmUpdateWorkerKey to ConfirmChangeWorkerAddress * Power actor: Export methods to CreateMiner and get miner counts (filecoin-project#868) * Power: Export CreateMiner * Power Actor: Export MinerCount and MinerConsensusCount * Update actors/power/src/lib.rs Co-authored-by: Alex <[email protected]> Co-authored-by: Alex <[email protected]> * Verifreg: Export AddVerifiedClient and GetClaims (filecoin-project#873) * Verifreg: Rename AddVerifierClientParams to AddVerifiedClientParams * Verifreg: Export AddVerifiedClient and GetClaims * Datacap actor: Modify exported methods (filecoin-project#909) * Datacap: Export Mint and Destroy * Datacap actor: Deprecate all internal methods * Datacap actor: Rename BalanceOf to Balance * Datacap actor: Add Granularity method * fix: comments on newly exported methods (filecoin-project#910) * Miner: Export method to GetPendingOwner * MarketNotifyDeal (filecoin-project#944) Co-authored-by: zenground0 <[email protected]> * Verifreg: Call AuthenticateMessage instead of validating siggys * Multisig: Do not export any functionality (filecoin-project#967) * use const for EX_DEAL_EXPIRED (filecoin-project#954) * Address review Co-authored-by: Alex <[email protected]> Co-authored-by: ZenGround0 <[email protected]> Co-authored-by: zenground0 <[email protected]> Co-authored-by: Shashank <[email protected]>

arajasek requested a review from anorth November 10, 2022 18:45

anorth requested changes Nov 11, 2022

View reviewed changes

arajasek added 2 commits November 14, 2022 19:10

Paych actor: Drop account req, use AuthenticateMessage to verify sigs

95822b5

Address review

2840559

arajasek force-pushed the asr/paych-accounts branch from aaf1837 to 2840559 Compare November 15, 2022 19:41

anorth mentioned this pull request Nov 15, 2022

Fix resolve_to_actor_id to check actor exists #846

Closed

anorth reviewed Nov 15, 2022

View reviewed changes

Address review

fcc43c4

arajasek force-pushed the asr/paych-accounts branch from 71194a8 to fcc43c4 Compare November 16, 2022 19:54

Merge branch 'integration/builtin-api' into asr/paych-accounts

42b7478

anorth approved these changes Nov 16, 2022

View reviewed changes

arajasek merged commit 564117f into integration/builtin-api Nov 16, 2022

arajasek deleted the asr/paych-accounts branch November 16, 2022 22:21

arajasek added a commit that referenced this pull request Nov 16, 2022

Paych actor: Drop account req, use AuthenticateMessage to verify sigs (…

13a9860

…#824) * Paych actor: Drop account req, use AuthenticateMessage to verify sigs * Address review * Address review

arajasek added a commit that referenced this pull request Nov 16, 2022

Paych actor: Drop account req, use AuthenticateMessage to verify sigs (…

8b4cae8

…#824) * Paych actor: Drop account req, use AuthenticateMessage to verify sigs * Address review * Address review

arajasek added a commit that referenced this pull request Nov 17, 2022

Paych actor: Drop account req, use AuthenticateMessage to verify sigs (…

5b96f2d

…#824) * Paych actor: Drop account req, use AuthenticateMessage to verify sigs * Address review * Address review

arajasek added a commit that referenced this pull request Nov 17, 2022

Paych actor: Drop account req, use AuthenticateMessage to verify sigs (…

b015d19

…#824) * Paych actor: Drop account req, use AuthenticateMessage to verify sigs * Address review * Address review

arajasek added a commit that referenced this pull request Nov 17, 2022

Paych actor: Drop account req, use AuthenticateMessage to verify sigs (…

4e700a2

…#824) * Paych actor: Drop account req, use AuthenticateMessage to verify sigs * Address review * Address review

arajasek added a commit that referenced this pull request Nov 21, 2022

Paych actor: Drop account req, use AuthenticateMessage to verify sigs (…

02ba857

…#824) * Paych actor: Drop account req, use AuthenticateMessage to verify sigs * Address review * Address review

arajasek added a commit that referenced this pull request Nov 23, 2022

Paych actor: Drop account req, use AuthenticateMessage to verify sigs (…

e743644

…#824) * Paych actor: Drop account req, use AuthenticateMessage to verify sigs * Address review * Address review

arajasek mentioned this pull request Nov 25, 2022

Audit and remove unwanted builtin-actor code for FVM #424

Open

arajasek added a commit that referenced this pull request Nov 27, 2022

Paych actor: Drop account req, use AuthenticateMessage to verify sigs (…

155dfe2

…#824) * Paych actor: Drop account req, use AuthenticateMessage to verify sigs * Address review * Address review

arajasek added a commit that referenced this pull request Nov 28, 2022

Paych actor: Drop account req, use AuthenticateMessage to verify sigs (…

7d2f8c3

…#824) * Paych actor: Drop account req, use AuthenticateMessage to verify sigs * Address review * Address review

arajasek added a commit that referenced this pull request Nov 28, 2022

Paych actor: Drop account req, use AuthenticateMessage to verify sigs (…

b54edd1

…#824) * Paych actor: Drop account req, use AuthenticateMessage to verify sigs * Address review * Address review

arajasek added a commit that referenced this pull request Nov 29, 2022

Paych actor: Drop account req, use AuthenticateMessage to verify sigs (…

79a5bc6

…#824) * Paych actor: Drop account req, use AuthenticateMessage to verify sigs * Address review * Address review

arajasek added a commit that referenced this pull request Nov 30, 2022

Paych actor: Drop account req, use AuthenticateMessage to verify sigs (…

c39312b

…#824) * Paych actor: Drop account req, use AuthenticateMessage to verify sigs * Address review * Address review

arajasek added a commit that referenced this pull request Nov 30, 2022

Paych actor: Drop account req, use AuthenticateMessage to verify sigs (…

382ab4c

…#824) * Paych actor: Drop account req, use AuthenticateMessage to verify sigs * Address review * Address review

arajasek added a commit that referenced this pull request Dec 5, 2022

Paych actor: Drop account req, use AuthenticateMessage to verify sigs (…

370bfa2

…#824) * Paych actor: Drop account req, use AuthenticateMessage to verify sigs * Address review * Address review

arajasek added a commit that referenced this pull request Dec 7, 2022

Paych actor: Drop account req, use AuthenticateMessage to verify sigs (…

5370afd

…#824) * Paych actor: Drop account req, use AuthenticateMessage to verify sigs * Address review * Address review

jennijuju added this to the Reenginering built-in actors API milestone Dec 9, 2022

arajasek added a commit that referenced this pull request Dec 11, 2022

Paych actor: Drop account req, use AuthenticateMessage to verify sigs (…

cd33132

…#824) * Paych actor: Drop account req, use AuthenticateMessage to verify sigs * Address review * Address review

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Paych actor: Drop account req, use AuthenticateMessage to verify sigs #824

Paych actor: Drop account req, use AuthenticateMessage to verify sigs #824

arajasek commented Nov 9, 2022

codecov-commenter commented Nov 9, 2022 •

edited

Loading

anorth left a comment

anorth Nov 11, 2022

arajasek Nov 15, 2022

anorth Nov 11, 2022

anorth Nov 11, 2022

anorth commented Nov 11, 2022

anorth Nov 15, 2022

anorth Nov 15, 2022

arajasek Nov 16, 2022

anorth Nov 15, 2022

Paych actor: Drop account req, use AuthenticateMessage to verify sigs #824

Paych actor: Drop account req, use AuthenticateMessage to verify sigs #824

Conversation

arajasek commented Nov 9, 2022

codecov-commenter commented Nov 9, 2022 • edited Loading

Codecov Report

anorth 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

anorth commented Nov 11, 2022

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

codecov-commenter commented Nov 9, 2022 •

edited

Loading