coinbase / mesh-sdk-go

Mesh Client Go SDK
Apache License 2.0
192 stars 135 forks source link

Handling account aliases on the Concordium blockchain (rosetta-cli) #422

Open bisgardo opened 2 years ago

bisgardo commented 2 years ago

Is your feature request related to a problem? Please describe.

On the Concordium blockchain we have the concept of account aliases, i.e. only the first 29 (of 32) bytes of an account address are significant. This confuses rosetta-cli check:data as it has no way of knowing if two addresses actually reference the same account. If an operation was performed for some address, it will observe that the balance of the "account" of the aliases changed for no reason.

Describe the solution you'd like

I've been trying to find a way to patch rosetta-cli (via rosetta-sdk-go) to translate all addresses into the "canonical" one. I've gotten it working by adding the following abomination of a hack into Syncer.processBlock:

@@ -187,6 +229,11 @@ func (s *Syncer) processBlock(
        }

        block := br.block
+       for i := range block.Transactions {
+               for j := range block.Transactions[i].Operations {
+                       canonicalizeAccountAddr(block.Transactions[i].Operations[j].Account)
+               }
+       }
        err = s.handler.BlockAdded(ctx, block)
        if err != nil {
                return err

where canonicalizeAccountAddr does a lookup using the Concordium SDK and replaces the address in-place.

I've not been able to find a proper place to implement the behavior as Syncer, StatefulSyncer, Fetcher, etc. are all structs and not interfaces. Helper is an interface type but it's not being constructed in a factory that can be overriden.

I'm not sure if monkey patching the block result is the correct solution, but it's the only one I've gotten to work (also tried BalanceStorage.AddingBlock and Parser.BalanceChanges but those seemed insufficient).

Describe alternatives you've considered

Patching all accounts in our Rosetta implementation. That would defeat the purpose of having aliases and also be prohibitively expensive.

Additional context

Any ideas of the correct way to implement this are greatly appreciated. I'll be happy to propose a PR with the necessary changes to for example support overriding the behavior of Syncer.

jingweicb commented 1 year ago

@bisgardo hi, thanks for raising up this question, if I understand correctly, the mapping method of canonicalizeAccountAddr will be same for all the accounts address in Concordium blockchain? the reason of these changes are aiming to record/using a same address in rosetta: check:dataand check:constructions?

bisgardo commented 1 year ago

Hi @jingweicb thanks for looking into this!

Yes canonicalizeAccountAddr works for any Concordium address. I didn't get check:constructions to work back when I tried it, but I don't think this issue would apply to it. The problem arises in the bookkeping part of check:data that checks recorded balances against Rosetta.

Example: Consider two distinct account addresses A and B that are actually aliases of the same account. Let's say the balance of this account is 100.

The bookkeeper has seen and recorded both A and B having balance 100. Later, it reads a block where A sends 10 to X. It now checks the balance of A via Rosetta and finds it to be 90 as expected. But next time it checks B and also finds a balance 90 it reports an error because it doesn't know that it's the same account: It hasn't seen any transactions that it thinks would affect B so it assumes that it's correct balance is still 100.

The solution above fixes the problem by hiding the concept of account aliases entirely. I think a better solution would do the mapping only when reading and writing the bookkeeper's state. That is, in pseudocode:

bookkeeper.write(address, balance)

becomes

bookkeeper.write(toCanonical(address), balance)

and similar for read. But as described above I didn't manage to implement a working solution for this.

bisgardo commented 1 year ago

Btw. you can see the solution we ended up using at https://github.com/coinbase/rosetta-cli/compare/master...Concordium:rosetta-cli:concordium.

Is a little more complex that described in the original issue because account aliases didn't exist from the beginning but was introduced in a protocol update. So instead of storing the canonical address, we store a mapping from the "zero" alias to the first observed address. For old accounts this is the only valid address, for new accounts it's the canonical alias.

But conceptually it's pretty much the same as it's still just a fixed mapping from one address to another.

jingweicb commented 1 year ago

@bisgardo thanks for your context, I am looking for a way to minimize the changes will circle back this week

bisgardo commented 1 year ago

Thank you @jingweicb. Any news? :)

jingweicb commented 1 year ago

@bisgardo , thanks for you patience! Sorry for reply later, our team are pretty busy recently.

Generally, we tend to handle the account aliases in rosetta-implementation level. Is there any way to record a unique address for each account in rosetta-implementation? We saw some blockchains have similar problems, they were able to map the alias to one address in /block and /account endpoints. Of course they do not have 16 millions address for each accounts, so maybe for them, solving in this way is much easier than Concordium Could you let us know the blockers to do this? So we could have a better understanding of this problem

bisgardo commented 1 year ago

Thanks @jingweicb, now it's my turn to apologize for the late response ;)

I don't think mapping the accounts in our Rosetta impl is feasible as it would make it stateful, which I would very much prefer it not to be. Mapping to the "zeroth" alias would be possible it not for the fact that old accounts don't have aliases as explained in a previous comment. But even if we could, I think users would be confused to have the addresses change in ways that aren't obvious to them and probably not sympathize with this being done only to satisfy a test tool. I don't feel like Rosetta should have an opinion on what alias to show, but rather show what's actually recorded in the blocks.

Does this make sense?

May I ask what is holding you back from allowing the mapping to done in the CLI? Note that I don't suggest implementing any kind of native support. The SDK change is only to allow us to implement it in our own fork.

jingweicb commented 1 year ago

@bisgardo thanks for your clarification

the reason why we do not want to change cli and sdk are 1, we wanna keep sdk and cli as generic as it could be, special logic is better to add in rosetta implemetatiom 2, we saw some rosetta-implemetation successfully handled alias problem and working in our production environments well 3, if we donot unify these alias in rosetta, at least limiting the numbers of alias it will finally block concordium in somewhere else

also rosetta implementation do not have to be stateless, and users will not see all the alias in our platform here is an example we found how they switch alias in rosetta, https://github.com/hashgraph/hedera-mirror-node/blob/main/hedera-mirror-rosetta/app/services/account_service.go

jingweicb commented 1 year ago

@bisgardo here are updates from my side the best way for Concordium blockchain is adding the accounts transfer logic in rosetta implementation, as it will request the minimum changes in related areas thanks for the suggestion, we will add call back functions to enable customized accounts transfer functions, but no ETA also,feel free to use your fork repo's test results