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

Relaxed address strength restrictions for legacy 20 byte addresses #772

Merged
merged 5 commits into from
Mar 3, 2022

Conversation

iramiller
Copy link
Contributor

@iramiller iramiller commented Mar 2, 2022

This PR upstreams the changes required for addressing #758 on the Provenance Blockchain.

By removing the assumption that a valid address would always be 32 bytes long per the current standard, the previous shorter 20 byte addresses can be used for accessing existing instances. All new instances will conform to the new 32 byte form.

(PR opened by request of @the-frey, also CC @channa-figure )

@iramiller iramiller requested a review from alpe as a code owner March 2, 2022 15:01
@codecov
Copy link

codecov bot commented Mar 2, 2022

Codecov Report

Merging #772 (ce034f0) into master (d4284fb) will increase coverage by 0.15%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #772      +/-   ##
==========================================
+ Coverage   58.47%   58.62%   +0.15%     
==========================================
  Files          49       49              
  Lines        5828     5830       +2     
==========================================
+ Hits         3408     3418      +10     
+ Misses       2170     2163       -7     
+ Partials      250      249       -1     
Impacted Files Coverage Δ
x/wasm/types/keys.go 51.11% <100.00%> (+16.22%) ⬆️
x/wasm/keeper/keeper.go 87.90% <0.00%> (+0.35%) ⬆️

@the-frey
Copy link
Contributor

the-frey commented Mar 2, 2022

Thanks @iramiller - keen to get this tested at our end :)

@faddat
Copy link
Contributor

faddat commented Mar 2, 2022

Many thanks @iramiller

Copy link
Member

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. Surprised if there are not other places that need to be adjusted.

I guess it is theoretically possible that a 32 byte address overlaps with a 20 bytes address (the first 20 bytes match the other one exactly) and this will cause some issues for prefix scans on the shorter one.

However, these are hashes and the chance of such a collision is something like 2^160 and currently technically impossible (harder than rewriting bitcoin's history)

@iramiller
Copy link
Contributor Author

We have been running with this change on our testnet... it was all that we required based on our testing... we have contracts with both length addresses now.

@alpe
Copy link
Contributor

alpe commented Mar 2, 2022

LGTM.
In the tests we use 32 byte addresses. It is an important design decision that 20 byte addresses should work, too. I am afraid that without code comments and/ or tests it would be easy to break in the future. If you have some capacity, please add some tests so that we are on the safe side. 🙏

@iramiller
Copy link
Contributor Author

Some more comments/tests coverage makes sense.

When I opened this PR I only wanted the two initial commits that we are actually running on Provenance pio-testnet-1 for clarity so as to not imply there were more pieces that we have verified directly than what we are actually using.

@channa-figure and I discussed the unit test angle this morning and it should be straight forward to add some extra comments and a test that asserts the 20 byte form is accepted here as well as the 32 byte version in keys_test.go)

@channa-figure
Copy link
Contributor

I added a new test and a test case to the keys_test.go file. I also left a comment with in the test for a little clarification.

Copy link
Contributor

@alpe alpe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice. Thanks for the contribution and the additional test. 🥇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants