During migration from legacy 20 byte address a 'no history for address' error occurs

[iramiller]

In testing 0.23 on a testnet with existing contract instance an error is thrown due to the way kvstore addresses are generated. Addresses are based on the current 32 byte length form, unfortunately some contracts were established with the prior 20 byte address form which is incompatible with this approach.

wasmd/x/wasm/types/keys.go

Line 90 in de38ea0

r := make([]byte, prefixLen+ContractAddrLen)

The above line always presumes the length will be 32 and it fails because it can't delete the history here:

wasmd/x/wasm/keeper/keeper.go

Line 578 in de38ea0

prefixStore := prefix.NewStore(ctx.KVStore(k.storeKey), types.GetContractCodeHistoryElementPrefix(contractAddr))

This occurred during testing on Provenance (using wasmd 0.23) with the following:

provenanced tx wasm migrate \
    tp15qkf84uddc0zjpkjrn64rln75w40plrtcq4kp3 \
    111 \
    '{}' \
    --chain-id pio-testnet-1 \
    --node tcp://rpc-0.test.provenance.io:26657 \
    [ ... ]

Error: rpc error: code = InvalidArgument desc = recovered: no history for tp15qkf84uddc0zjpkjrn64rln75w40plrtcq4kp3
stack:
goroutine 764913 [running]:
runtime/debug.Stack()
    runtime/debug/stack.go:24 +0x65

[iramiller]

One potential solution here is to use:

	addrLen := len(contractAddr)
	r := make([]byte, prefixLen+addrLen)

Length assertions could be inserted to require 20 or 32 byte address forms (which are the only two allowed) however if this is done additional refactoring will be required as the methods do not currently return errors and a panic isn't really desirable.

As the purpose of this key is to lookup existing cached data ... if the address were invalid here then there would be no data matched so additional validity checks are likely not needed.

[alpe]

Can you give more context why you need 20 byte contract addresses in your scenario?
A mix of both address sizes adds some complexity that I would avoid. You can either migrate the 20 byte addresses to 32 bytes or set the ContractAddrLen to 20 bytes.
The later one is not possible currently without changing code as it is a constant. Just open an issue/ PR for this.

The contract address is deterministic and based on codeID and contractID. Unfortunately the contractID is not persisted but can be found with some smarter than brute force algorithm for a migration.

[the-frey]

By migration you mean at the contract level right? We will run into this issue on Juno. as well (as I think we have 3-4 length 20 contracts instantiated when the chain first launched.

My concern is that if migrations to contracts are required, possibly either the original instantiators won't be contactable, or they won't have set an admin, so the contracts won't be migrate-able.

If it's at binary level then that's less of a concern.

[alpe]

Sorry, migration was ambiguous. I was referring to sdk upgrade . Contracts don't have access to the store outside their namespace.
This address "update" would also affect balances and others places where the contract address is used.

[the-frey]

Right, I did wonder. So in summary if we have 20 byte contract addresses then before moving to mainline wasmd or wasmd 0.23 we would need a migration that ensures every existing contract with a 20 byte address is changed to 32?

[alpe]

To summarize, I can see these 3 options:

A) "upgrade" all contracts to 32 byte addresses and go with vanilla wasmd
B) we make ContractAddrLen a var so that every team can decide and set their contract address length (32byte would be default)
C) have a mixed setup with 20 and 32 byte addresses as @iramiller has in their testnet currently which require changes to vanilla wasmd

As mentioned before, C) is not my preferred option as it adds complexity.

[alpe]

btw @iramiller thanks for bringing this topic up. Happy to see this discussion.

[iramiller]

The contract address is well known and used in several frontend applications... so while we could programmatically shift the state to a new 32 byte address that would break the application dependencies above the blockchain if we modify it. In this case there is a consortium of banks that all agreed on it. Certainly do able but the ask is to avoid that because these kinds of changes take more time than they should.

My fears about not supporting the 32 byte form are not well defined but are based on suspicions that future applications over IBC/interchain may see difficulties. By removing the constraint that the address will be 32 in length then existing ones continue to work and all future ones can be the proper 32 byte form.

[ethanfrey]

To summarize, I can see these 3 options:

A) "upgrade" all contracts to 32 byte addresses and go with vanilla wasmd

Addresses are stored in many places and we cannot change them. Wasmd upgrades must work with immutable contracts.

B) we make ContractAddrLen a var so that every team can decide and set their contract address length (32byte would be default)

They don't want 20 bytes. They want 32, but want backwards support for 20 bytes. Note that various components in the sdk (notably ibc) choked on 32 bytes addresses when Provenance and Juno made these choices.

C) have a mixed setup with 20 and 32 byte addresses as @iramiller has in their testnet currently which require changes to vanilla wasmd

As mentioned before, C) is not my preferred option as it adds complexity.

It is more complex for wasmd, but it is needed complexity and part of the follow up from our decision to move to 32 byte contract addresses (which I think everyone on this thread commented on and supported). I agree with @iramiller that we need to support this on the sdk side.

I also know that Alex is very busy with Tgrade work right now, but I would ask if there is interest in an external PR to add this as this is a critical fix needed for both your chains.

[the-frey]

👍 to everything in that summary.

I'd love to pitch in, but I think my knowledge of the wasmd/SDK internals is likely not up to the task 😞

[iramiller]

We have a fork with this change which we are running now on our testnet for evaluation... it would be straight forward to contribute this solution... in addition if this solution needs to develop further we are interested in helping out cosmwasm with testing these changes and we would want to see those included in our network before we cut our next release.

[channa-figure]

Here is a compare link for what we did to solve this issue. This is the change @iramiller was referring to.

v0.22.0...provenance-io:migration-address-length

[albertchon]

What is the downside for a fork to just change the ContractAddrLen to 20 bytes?

Why was this change made to begin with? Can't seem to find an explanation for this

[iramiller]

Originally the sdk assigned 20 byte addresses but a switch to 32 was made for new addresses based on new key types. Existing addresses were maintained.

The cosmwasm system is taking this differently with the current implementation breaking all existing contracts when it blocks support for existing addresses.

More details here: https://github.com/cosmos/cosmos-sdk/blob/master/docs/architecture/adr-028-public-key-addresses.md

[ethanfrey]

@albertchon when preparing the PR to update to 0.45 we had number of discussions of the two teams we knew using forks with 0.45 - Juno and Provenance.

You can see the discussion here on a PR proposal from the Juno team.

This was also heavily promoted by the Cosmos SDK team. Well before 0.43 came out, Robert was asking me to migrate to the new module account hash algorithm.

[ethanfrey]

I agree with @iramiller that we should support both and new addresses are 32 bytes

[ethanfrey]

Note: the first 2 commits of this PR #763 seem to address a few items required for this issue

[the-frey]

@channa-figure / @iramiller would you put in a PR to wasmd with your solution? On the current Juno testnet we've moved off our fork and onto mainline wasmd, so we can test a point release/pre-release as well if it's in this repo. Otherwise a bit of faff to change to your fork, test, then change back to mainline 🙂

[ethanfrey]

Closed by #772