Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
bisgardo opened this issue Jul 4, 2022 · 9 comments · May be fixed by #423
Open

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

bisgardo opened this issue Jul 4, 2022 · 9 comments · May be fixed by #423
Labels
enhancement New feature or request

Comments

@bisgardo
Copy link

bisgardo commented Jul 4, 2022

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.

@bisgardo bisgardo added the enhancement New feature or request label Jul 4, 2022
@bisgardo bisgardo linked a pull request Jul 6, 2022 that will close this issue
@jingweicb
Copy link
Contributor

@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:data and check:constructions ?

@bisgardo
Copy link
Author

bisgardo commented Jan 21, 2023

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
Copy link
Author

Btw. you can see the solution we ended up using at coinbase/mesh-cli@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
Copy link
Contributor

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

@bisgardo
Copy link
Author

bisgardo commented Feb 9, 2023

Thank you @jingweicb. Any news? :)

@jingweicb
Copy link
Contributor

jingweicb commented Feb 13, 2023

@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
Copy link
Author

bisgardo commented Feb 20, 2023

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
Copy link
Contributor

@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
Copy link
Contributor

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging a pull request may close this issue.

2 participants