540 change runtime resolve address to return an actorid #549

lyswifter · 2022-08-16T02:08:08Z

resolves #540

lyswifter · 2022-08-16T02:16:15Z

In some places, the code Address::new_id are not useful for network beside mainnet, the usage of NETWORK_DEFAULT maybe not reasonable I think.

…rn-an-actorid

anorth · 2022-08-16T04:51:32Z

That NETWORK_DEFAULT value looks like it's supposed to be set by a build flag. This implementation of Address differs from the Go one that it copied. I don't think we should worry about it here, but I can see potential problems for testnets.

anorth · 2022-08-16T04:57:51Z

actors/market/src/lib.rs

@@ -234,7 +234,7 @@ impl Actor {
        })?;

        let code_id = rt
-            .get_actor_code_cid(&provider)
+            .get_actor_code_cid(&Address::new_id(provider))


Thanks, we've ended up with a bunch of places now where we resolve to an ID then immediately wrap it again for another syscall.

Here I think we should change get_actor_code_cid to take an ActorID too. You'll have to wrap it in an address one level lower to meet the SDK, but I'll try to get the SDK changed to match too, later.

anorth · 2022-08-16T04:59:01Z

actors/market/src/lib.rs

@@ -244,7 +244,7 @@ impl Actor {
            ));
        }

-        let (_, worker, controllers) = request_miner_control_addrs(rt, provider)?;
+        let (_, worker, controllers) = request_miner_control_addrs(rt, Address::new_id(provider))?;


Change request_miner_control_addrs to accept an ActorID and wrap it lower down.

anorth · 2022-08-16T05:00:52Z

actors/market/src/lib.rs

            let mut client_lockup =
                total_client_lockup.get(&client_id).cloned().unwrap_or_default();
            client_lockup += deal.proposal.client_balance_requirement();

-            let client_balance_ok = msm.balance_covered(client, &client_lockup).map_err(|e| {
+            let client_balance_ok = msm.balance_covered(Address::new_id(client_id), &client_lockup).map_err(|e| {


Same here, it's a good prompt to only use resolved addresses for lookups

anorth · 2022-08-16T05:01:49Z

actors/miner/src/lib.rs

@@ -4025,7 +4025,7 @@ where
        ));
    }

-    Ok(resolved)
+    Ok(Address::new_id(resolved))


Probably just return the ActorID here, unless that causes lots of churn at caller.s

actors/miner/src/lib.rs

anorth · 2022-08-16T05:03:49Z

actors/paych/src/lib.rs

@@ -147,7 +147,7 @@ impl Actor {
                sv.channel_addr
            )
        })?;
-        if pch_addr != svpch_id_addr {
+        if pch_addr != Address::new_id(svpch_id_addr) {


An alternative here, where we know we have an ID address, is
pch_addr.id().unwrap() == svpch_id

The unwrap is a bit unpleasant and kinda points to us later changing message.receiver() to return the actor ID too. But it's probably better than wrapping all the time. I could be convinced otherwise. Any thoughts @ZenGround0 ?

anorth · 2022-08-16T05:04:28Z

runtime/src/builtin/shared.rs

@@ -25,17 +26,19 @@ where
 {
    // if we are able to resolve it to an ID address, return the resolved address
    if let Some(addr) = rt.resolve_address(address) {
-        return Ok(addr);
+        return Ok(Address::new_id(addr));


Could we change this to return ActorID? Would that cause lots of noise at callers?

lyswifter · 2022-08-16T06:20:24Z

I had changed get_actor_code_cid to receive an ActorID as input, you could have a check.
By the way I don't know why the first CI was failed for this PR, could you give me some tips?

anorth

Thanks, getting close now. There are still some wrappings we could purge, but I'm happy to do them incrementally after landing this.

anorth · 2022-08-16T21:36:13Z

actors/init/src/lib.rs

+            Err(e) => {
+                return Err(actor_error!(forbidden; "caller {} transfer to id address is not support, {}", caller_addr, e))
+            }
+        }


Replace this with rt.message().caller().id().unwrap(). The syscall guarantees this will work. The same goes for message().receiver() and a number of other places where the runtime makes this promise.

(We might later consider caller() returning an ID, but that would be a larger change)

anorth · 2022-08-16T21:37:17Z

actors/market/src/lib.rs

            let mut client_lockup =
                total_client_lockup.get(&client_id).cloned().unwrap_or_default();
            client_lockup += deal.proposal.client_balance_requirement();

-            let client_balance_ok = msm.balance_covered(client, &client_lockup).map_err(|e| {
+            let client_balance_ok = msm.balance_covered(Address::new_id(client_id), &client_lockup).map_err(|e| {


Change balance_covered to accept an ActorID

anorth · 2022-08-16T21:38:28Z

actors/market/src/lib.rs

@@ -1307,17 +1306,19 @@ where
        .resolve_address(addr)
        .ok_or_else(|| actor_error!(illegal_argument, "failed to resolve address {}", addr))?;

+    let nominal_addr = Address::new_id(nominal);


Move this down to just before its first use.

anorth · 2022-08-16T21:40:33Z

actors/multisig/src/lib.rs

@@ -306,6 +306,8 @@ impl Actor {
            )
        })?;

+        let resolved_new_signer = Address::new_id(resolved_new_signer);


Change is_signer to take an ActorID instead, then just inline this new_id call when pushing onto st.signers.

anorth · 2022-08-16T21:44:32Z

runtime/src/runtime/fvm.rs

+                Err(e) => {
+                    return Err(actor_error!(forbidden; "caller {} transfer to id address is not support, {}", caller_addr, e))
+                }
+            }


Just unwrap(), the VM guarantees this

anorth · 2022-08-16T21:45:18Z

runtime/src/test_utils.rs

+        match self.get_id_address(address) {
+            None => None,
+            Some(addr) => {
+                if let &Payload::ID(id) = addr.payload() {


This can't fail, can it? Just unwrap()

maybe for some tests unable_to_resolve_client_address, we should reserve this. How do you think?

I agree we need the match block for get_id_address. If it fails to resolve, it should return none. But if it returns Some then the value must be an ID address that we can unwrap.

anorth · 2022-08-16T21:46:50Z

test_vm/src/lib.rs

-        self.v.normalize_address(addr)
+    fn resolve_address(&self, addr: &Address) -> Option<ActorID> {
+        if let Some(normalize_addr) = self.v.normalize_address(addr) {
+            if let &Payload::ID(id) = normalize_addr.payload() {


This maybe also need reserve for some tests

Same. We need the outer if let, but the inner one can't fail.

lyswifter · 2022-08-17T11:27:02Z

you can take a look at those changes

anorth

Thanks, this is looking good to me now. I will request a second look from @ZenGround0. We don't need to fully push through IDs everywhere and I think this provides a good balance.

anorth · 2022-08-17T22:11:16Z

runtime/src/test_utils.rs

+        match self.get_id_address(address) {
+            None => None,
+            Some(addr) => {
+                if let &Payload::ID(id) = addr.payload() {


I agree we need the match block for get_id_address. If it fails to resolve, it should return none. But if it returns Some then the value must be an ID address that we can unwrap.

anorth · 2022-08-17T22:11:58Z

test_vm/src/lib.rs

-        self.v.normalize_address(addr)
+    fn resolve_address(&self, addr: &Address) -> Option<ActorID> {
+        if let Some(normalize_addr) = self.v.normalize_address(addr) {
+            if let &Payload::ID(id) = normalize_addr.payload() {


Same. We need the outer if let, but the inner one can't fail.

anorth · 2022-08-17T22:13:53Z

Build is failing due to formatting. Run cargo fmt to fix

…resolve_address-to-return-an-actorid

anorth

Please don't merge until @ZenGround0 has time to take a look.

ZenGround0

The only thing I'm worried about is the change to the test, if the test isn't actually broken this is good to go. The other comments are all nitpicking. It would be very nice if you change all ActorID typed variables and related function names to have names that are not x_addr but instead x_id since addresses and actor ids are distinct.

ZenGround0 · 2022-08-25T03:10:50Z

actors/market/src/lib.rs

@@ -53,14 +53,14 @@ fil_actors_runtime::wasm_trampoline!(Actor);

 fn request_miner_control_addrs<BS, RT>(
    rt: &mut RT,
-    miner_addr: Address,
+    miner_addr: ActorID,


ZenGround0 · 2022-08-25T03:15:52Z

actors/market/src/lib.rs

@@ -235,7 +235,7 @@ impl Actor {

        let code_id = rt
            .get_actor_code_cid(&provider)


nit: change name above to provider_id

ZenGround0 · 2022-08-25T03:21:33Z

runtime/src/util/chaos/types.rs

@@ -25,7 +25,7 @@ pub struct CreateActorArgs {
 /// Holds the response of a call to runtime.ResolveAddress
 #[derive(Serialize_tuple, Deserialize_tuple)]
 pub struct ResolveAddressResponse {
-    pub address: Address,
+    pub address: ActorID,


pub id: ActorID

ZenGround0 · 2022-08-25T03:23:54Z

actors/market/src/state.rs

@@ -528,15 +529,17 @@ where
    // Return true when the funds in escrow for the input address can cover an additional lockup of amountToLock
    pub(super) fn balance_covered(
        &self,
-        addr: Address,


nit: Since this is keyed by address it would make sense for this code to expect an address, not change its internal logic to better adapt to the caller.

ZenGround0 · 2022-08-25T03:26:20Z

actors/market/tests/market_actor_test.rs

    deal.verified_deal = true;

    // add funds for cient using it's BLS address -> will be resolved and persisted
-    add_participant_funds(&mut rt, client_bls, deal.client_balance_requirement());


Blocking: What's going on here? I don't think this change should break this test?

ZenGround0 · 2022-08-25T03:33:15Z

actors/paych/src/lib.rs

@@ -147,7 +147,7 @@ impl Actor {
                sv.channel_addr
            )
        })?;
-        if pch_addr != svpch_id_addr {
+        if pch_addr.id().unwrap() != svpch_id_addr {


nit: its a bit nicer to make svpch_id_addr an address to do the comparison because then there is no code path to panic

ALso svpch_id not svpch_id_addr

ZenGround0 · 2022-08-25T03:33:43Z

actors/reward/src/lib.rs

@@ -158,7 +159,7 @@ impl Actor {
        // if this fails, we can assume the miner is responsible and avoid failing here.
        let reward_params = ext::miner::ApplyRewardParams { reward: total_reward.clone(), penalty };
        let res = rt.send(
-            miner_addr,
+            Address::new_id(miner_addr),


ZenGround0 · 2022-08-25T03:35:26Z

actors/verifreg/src/lib.rs

@@ -436,6 +444,8 @@ impl Actor {
            )


change the name of the above resolve_to_id_addr to resolve_to_id or resolve_addr_to_id or resolve_to_actor_id

change comment "failed to resolve addr {} to ID"

ZenGround0 · 2022-08-25T03:36:23Z

runtime/src/builtin/shared.rs

-            address,
-        )
-    })
+    if let Some(addr) = rt.resolve_address(address) {


ZenGround0 · 2022-08-25T03:42:54Z

actors/multisig/src/state.rs

-    pub fn is_signer(&self, address: &Address) -> bool {
-        self.signers.contains(address)
+    pub fn is_signer(&self, id: &ActorID) -> bool {
+        self.signers.contains(&Address::new_id(*id))


I think this is to act as a level of protection to ensure that we don't do bogus checks of non id addresses against addresses that are resolved to id addresses.

If we go this route why not also do this on purge_approvals? This also implies we should add st accessors for push, retain etc that take ActorIDs and do the address wrapping internal to the state.

You could also argue that this is unnecessary and then this could go back to taking in an address. It would be good to be consistent.

…slove_to_id_addr to reslove_to_actor_id

…resolve_address-to-return-an-actorid

lyswifter · 2022-08-26T06:10:24Z

@ZenGround0 I had do some modifies to your suggestions,you can have a review.

ZenGround0 · 2022-08-28T16:11:16Z

Changes look good but the test is still modified, I'm guessing its failing and need to investigate why before approving.

ZenGround0 · 2022-08-28T16:31:01Z

actors/market/tests/market_actor_test.rs

@@ -729,7 +729,8 @@ fn provider_and_client_addresses_are_resolved_before_persisting_state_and_sent_t
    let end_epoch = start_epoch + 200 * EPOCHS_IN_DAY;
    rt.set_epoch(start_epoch);

-    let mut deal = generate_deal_proposal(client_bls, provider_bls, start_epoch, end_epoch);
+    let mut deal =
+        generate_deal_proposal(client_resolved, provider_resolved, start_epoch, end_epoch);


Undo all changes to this file. The point of this test is to use the bls addresses to make sure the actor resolves things correctly. Your code change doesn't break this test.

ZenGround0 · 2022-08-28T16:40:58Z

actors/market/tests/publish_storage_deals_failures.rs

@@ -374,7 +374,7 @@ fn fail_to_resolve_provider_address() {

    let mut rt = setup();
    let mut deal = generate_deal_proposal(CLIENT_ADDR, PROVIDER_ADDR, start_epoch, end_epoch);
-    deal.provider = new_bls_addr(100);


Undo this change as well please.

lyswifter · 2022-08-29T02:23:26Z

I did some fixes and revert the client and provider address to its original, you can take a check @ZenGround0

Co-authored-by: lyswifter <ly70835@163.com>

…#549) Co-authored-by: lyswifter <ly70835@163.com>

lyswifter added 2 commits August 15, 2022 14:26

reslove address to ActorID and update some usages

4b79fea

reconstruct id_address from actorID in some places

c19edfa

lyswifter linked an issue Aug 16, 2022 that may be closed by this pull request

Change Runtime resolve_address to return an ActorID #540

Closed

lyswifter and others added 5 commits August 16, 2022 10:46

reslove code lint problems

9b1b2ef

remove return None for code lint

2a64d40

Merge branch 'master' into 540-change-runtime-resolve_address-to-retu…

1599d31

…rn-an-actorid

reslove reimport type

99c2d3f

fix return None for code lint

dcc6811

anorth reviewed Aug 16, 2022

View reviewed changes

change get_actor_code_cid to receive actorID directly

d014b96

anorth reviewed Aug 16, 2022

View reviewed changes

lyswifter added 2 commits August 17, 2022 11:24

using actorid instead of address for many places

2638198

revert a little mistake

34abc50

anorth reviewed Aug 17, 2022

View reviewed changes

anorth requested a review from ZenGround0 August 17, 2022 22:12

lyswifter added 2 commits August 18, 2022 08:34

Merge remote-tracking branch 'origin/master' into 540-change-runtime-…

fedb59d

…resolve_address-to-return-an-actorid

with cargo fmt to fix code format

876df19

jennijuju requested a review from anorth August 19, 2022 02:24

anorth approved these changes Aug 25, 2022

View reviewed changes

ZenGround0 requested changes Aug 25, 2022

View reviewed changes

lyswifter added 4 commits August 26, 2022 10:19

modify some params name from addr to id; change function name from re…

a3af2eb

…slove_to_id_addr to reslove_to_actor_id

Merge remote-tracking branch 'origin/master' into 540-change-runtime-…

22ad7db

…resolve_address-to-return-an-actorid

change error message to showing actor id

21be994

cargo fmt

3007380

ZenGround0 approved these changes Aug 28, 2022

View reviewed changes

lyswifter added 2 commits August 29, 2022 09:58

revert some test case to its origin

3ad0b45

cargo fmt

a2c133e

revert address in test

6dde29a

anorth enabled auto-merge (squash) August 29, 2022 02:38

anorth merged commit d19f9ff into master Aug 29, 2022

anorth deleted the 540-change-runtime-resolve_address-to-return-an-actorid branch August 29, 2022 03:03

Stebalien pushed a commit that referenced this pull request Aug 31, 2022

Change runtime resolve address to return an ActorID (#549)

97ff14e

Co-authored-by: lyswifter <ly70835@163.com>

shamb0 pushed a commit to shamb0/builtin-actors that referenced this pull request Jan 31, 2023

Change runtime resolve address to return an ActorID (filecoin-project…

c72c178

…#549) Co-authored-by: lyswifter <ly70835@163.com>

		@@ -235,7 +235,7 @@ impl Actor {

		let code_id = rt
		.get_actor_code_cid(&provider)

540 change runtime resolve address to return an actorid #549

540 change runtime resolve address to return an actorid #549

Conversation

lyswifter commented Aug 16, 2022 • edited by jennijuju Loading

lyswifter commented Aug 16, 2022

anorth commented Aug 16, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lyswifter commented Aug 16, 2022

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

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

lyswifter commented Aug 17, 2022

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

anorth commented Aug 17, 2022

anorth left a comment

Choose a reason for hiding this comment

ZenGround0 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lyswifter commented Aug 26, 2022

ZenGround0 commented Aug 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lyswifter commented Aug 29, 2022

lyswifter commented Aug 16, 2022 •

edited by jennijuju

Loading