Compatibility with the ARM architecture #8396

RiccardoM · 2021-01-20T12:58:37Z

Description

This PR adds the compatibility with the ARM architecture, by casting math.MaxInt64, math.MaxUint32 and math.MaxUint64 to uint64, uint32 and uint64 respectively.

It also adds a series of tests to make sure that the build is successful on a list of common platforms (386, arm, arm64 and amd64).

The aim of this PR is to make the Cosmos SDK compatible with the ARM architecture. This is often used in single board computers like the Raspberry, which are often used by hobbyists to experiment with projects. By making the Cosmos SDK ARM-compatible, we could allow such people to get started with it by running a node on their Raspberry before moving to a safer environment.

Also, this might be particularly useful to all the Cosmos SDK-based projects that are currently running a testnet and would like to give their validators the opportunity to run a node in-house on the above-mentioned boards.

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer
Review Codecov Report in the comment section below once CI passes

This PR adds the compatibility with the ARM architecture, by casting math.MaxInt64, math.MaxUint32 and math.MaxUint64 to uint64, uint32 and uint64 respectively.

x/ibc/testing/chain.go

.github/workflows/test.yml

Co-authored-by: Alessio Treglia <quadrispro@ubuntu.com>

baseapp/options.go

store/rootmulti/store.go

x/ibc/applications/transfer/module.go

Co-authored-by: Alessio Treglia <quadrispro@ubuntu.com>

alessio · 2021-01-20T14:10:19Z

Why are you casting math.MaxInt64 to uint64 (unsigned)?

RiccardoM · 2021-01-20T14:12:05Z

Why are you casting math.MaxInt64 to uint64 (unsigned)?

Cause the left side of the comparison is an uint64, so I figured out that was the correct type to have. Also, since math.MaxInt64 < math.MaxUint64, I thought that conversion would never give any problem

alessio · 2021-01-20T14:38:25Z

since math.MaxInt64 < math.MaxUint64, I thought that conversion would never give any problem

You need to cast to ensure it won't ever be a problem. However, what exactly is the problem you're trying to solve here? Does the compiler emit warnings/errors when building for arm64? And if, so can you post the buildlog please?

RiccardoM · 2021-01-20T15:09:19Z

since math.MaxInt64 < math.MaxUint64, I thought that conversion would never give any problem

You need to cast to ensure it won't ever be a problem. However, what exactly is the problem you're trying to solve here? Does the compiler emit warnings/errors when building for arm64? And if, so can you post the buildlog please?

This is only the first error that raises. Once you solve that, you get similar ones for the rest of the cases I have fixed:

$ GOOS=linux GOARCH=arm LEDGER_ENABLED=false make build
make[1]: Entering directory '/sda1/Coding/Cosmos/Cosmos-SDK'
go build -mod=readonly -tags "netgo" -ldflags '-X github.com/cosmos/cosmos-sdk/version.Name=sim -X github.com/cosmos/cosmos-sdk/version.AppName=simd -X github.com/cosmos/cosmos-sdk/version.Version=goz-phase-1-1151-g3d2896a2e -X github.com/cosmos/cosmos-sdk/version.Commit=3d2896a2e43eeb8ad9d157ab12c13936a54cd906 -X "github.com/cosmos/cosmos-sdk/version.BuildTags=netgo," -w -s' -trimpath -o /sda1/Coding/Cosmos/Cosmos-SDK/build/ ./...
# github.com/cosmos/cosmos-sdk/store/rootmulti
store/rootmulti/store.go:716:4: constant 9223372036854775807 overflows int
make[1]: *** [Makefile:110: build] Error 2

store/rootmulti/store.go

x/ibc/applications/transfer/module.go

Co-authored-by: Alessio Treglia <quadrispro@ubuntu.com>

Makefile

codecov · 2021-01-27T09:16:36Z

Codecov Report

Merging #8396 (6decdaf) into master (fd04f41) will not change coverage.
The diff coverage is 75.00%.

@@           Coverage Diff           @@
##           master    #8396   +/-   ##
=======================================
  Coverage   61.47%   61.47%           
=======================================
  Files         630      630           
  Lines       36398    36398           
=======================================
  Hits        22377    22377           
  Misses      11671    11671           
  Partials     2350     2350

Impacted Files	Coverage Δ
store/rootmulti/store.go	`66.05% <0.00%> (ø)`
snapshots/store.go	`74.31% <100.00%> (ø)`
x/ibc/applications/transfer/module.go	`60.68% <100.00%> (ø)`
x/ibc/core/02-client/types/client.go	`72.22% <100.00%> (ø)`

.github/workflows/test.yml

…ilation

@RiccardoM

From: #8396 Thanks: @RiccardoM for the original patch.

odeke-em

Howdy @RiccardoM, thank you for this change! I have a question about what looks likes now an increase for height, which I believe would involve security changes and perhaps would first go through a design change; this change is related to casting but not changing limits of values; we also need a test to prevent such regressions.

odeke-em · 2021-01-27T21:42:01Z

store/rootmulti/store.go

@@ -711,9 +711,9 @@ func (rs *Store) Restore(
 	if height == 0 {
 		return sdkerrors.Wrap(sdkerrors.ErrLogic, "cannot restore snapshot at height 0")
 	}
-	if height > math.MaxInt64 {
+	if height > uint64(math.MaxUint64) {


Oh oh, now height can be more than math.MaxInt64 aka >9223372036854775808, up until (1<<64 - 1) aka 18446744073709551615. That seems like a breaking change that's not related to ARM compatibility. The error message doesn't even match this check, plus I don't think there was a test to prevent such a regression. Please advise or revert this check.

This was changed here by @alessio. I think he should be able to answer this better. The commit was b34e327

This should fix it #8466 - @odeke-em

See #8396 (comment)

See #8396 (comment) Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

From: #8396 #8466

store/multistore: revert a height limit increase from #8396 See #8396 (comment)

@RiccardoM

From: #8396 #8466 Thanks: @RiccardoM for the original patch.

See cosmos/cosmos-sdk#8396 (comment) Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

Compatibility with the ARM architecture

72d9dc7

This PR adds the compatibility with the ARM architecture, by casting math.MaxInt64, math.MaxUint32 and math.MaxUint64 to uint64, uint32 and uint64 respectively.

RiccardoM requested review from aaronc, alessio and alexanderbez as code owners January 20, 2021 12:58

Trying to fix workflow

1067651

alessio reviewed Jan 20, 2021

View reviewed changes

x/ibc/testing/chain.go Outdated Show resolved Hide resolved

alessio reviewed Jan 20, 2021

View reviewed changes

.github/workflows/test.yml Outdated Show resolved Hide resolved

alessio reviewed Jan 20, 2021

View reviewed changes

.github/workflows/test.yml Outdated Show resolved Hide resolved

Update x/ibc/testing/chain.go

b514e55

Co-authored-by: Alessio Treglia <quadrispro@ubuntu.com>

alessio reviewed Jan 20, 2021

View reviewed changes

baseapp/options.go Outdated Show resolved Hide resolved

Deleted useless comments

cc81eb1

alessio reviewed Jan 20, 2021

View reviewed changes

store/rootmulti/store.go Outdated Show resolved Hide resolved

alessio reviewed Jan 20, 2021

View reviewed changes

store/rootmulti/store.go Outdated Show resolved Hide resolved

alessio reviewed Jan 20, 2021

View reviewed changes

x/ibc/applications/transfer/module.go Show resolved Hide resolved

RiccardoM and others added 3 commits January 20, 2021 15:09

Update store/rootmulti/store.go

54b2692

Co-authored-by: Alessio Treglia <quadrispro@ubuntu.com>

Update store/rootmulti/store.go

676582b

Co-authored-by: Alessio Treglia <quadrispro@ubuntu.com>

Update x/ibc/applications/transfer/module.go

39119cb

Co-authored-by: Alessio Treglia <quadrispro@ubuntu.com>

Merge branch 'master' into arm-compilation

7fdd9cb

alessio reviewed Jan 20, 2021

View reviewed changes

store/rootmulti/store.go Outdated Show resolved Hide resolved

Update store/rootmulti/store.go

b34e327

alessio reviewed Jan 20, 2021

View reviewed changes

x/ibc/applications/transfer/module.go Outdated Show resolved Hide resolved

alessio and others added 4 commits January 20, 2021 16:29

Update x/ibc/applications/transfer/module.go

c4b8a0a

Merge branch 'master' into arm-compilation

33932ef

Update .github/workflows/test.yml

9f1f3ba

Co-authored-by: Alessio Treglia <quadrispro@ubuntu.com>

Update .github/workflows/test.yml

d97ed62

Co-authored-by: Alessio Treglia <quadrispro@ubuntu.com>

alessio approved these changes Jan 25, 2021

View reviewed changes

alessio added the backports/0.41.x (Stargate) label Jan 27, 2021

alessio reviewed Jan 27, 2021

View reviewed changes

Makefile Outdated Show resolved Hide resolved

alessio reviewed Jan 27, 2021

View reviewed changes

Makefile Outdated Show resolved Hide resolved

Update Makefile

ddf2243

alessio requested review from fdymylja, jgimeno and sahith-narahari January 27, 2021 10:03

fdymylja approved these changes Jan 27, 2021

View reviewed changes

Merge branch 'master' into arm-compilation

d687bc4

jgimeno reviewed Jan 27, 2021

View reviewed changes

.github/workflows/test.yml Outdated Show resolved Hide resolved

RiccardoM added 2 commits January 27, 2021 11:49

Fixed CI

260ad25

Merge remote-tracking branch 'riccardo/arm-compilation' into arm-comp…

6decdaf

…ilation

jgimeno approved these changes Jan 27, 2021

View reviewed changes

alessio merged commit e2f510a into cosmos:master Jan 27, 2021

alessio pushed a commit that referenced this pull request Jan 27, 2021

Compatibility with the ARM architecture

6ad11b6

From: #8396 Thanks: @RiccardoM for the original patch.

alessio mentioned this pull request Jan 27, 2021

[backport/v0.41.x]: Compatibility with the ARM architecture #8450

Merged

9 tasks

RiccardoM deleted the arm-compilation branch January 27, 2021 11:27

odeke-em reviewed Jan 27, 2021

View reviewed changes

alessio pushed a commit that referenced this pull request Jan 28, 2021

partially revert #8396

3b92368

See #8396 (comment)

alessio mentioned this pull request Jan 28, 2021

store/multistore: revert a height limit increase from #8396 #8466

Merged

9 tasks

alessio pushed a commit that referenced this pull request Jan 28, 2021

store/multistore: revert a height limit increase from #8396

6874114

See #8396 (comment)

mergify bot added a commit that referenced this pull request Jan 29, 2021

store/multistore: revert a height limit increase from #8396 (#8466)

3b8e0f9

See #8396 (comment) Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

alessio pushed a commit that referenced this pull request Feb 1, 2021

compatibility with ARM architectures

c06a691

From: #8396 #8466

clevinson mentioned this pull request Feb 10, 2021

v0.41.1 Tracking Issue #8564

Closed

23 tasks

alessio pushed a commit that referenced this pull request Feb 17, 2021

From: #8466

8092989

store/multistore: revert a height limit increase from #8396 See #8396 (comment)

alessio pushed a commit that referenced this pull request Feb 17, 2021

[backport/v0.41.x]: Compatibility with the ARM architecture (#8450)

d685701

From: #8396 #8466 Thanks: @RiccardoM for the original patch.

daeMOn63 pushed a commit to fetchai/cosmos-sdk that referenced this pull request Aug 19, 2021

store/multistore: revert a height limit increase from #8396 (#8466)

1315caf

See cosmos/cosmos-sdk#8396 (comment) Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

daeMOn63 pushed a commit to fetchai/cosmos-sdk that referenced this pull request Aug 20, 2021

store/multistore: revert a height limit increase from #8396 (#8466)

036a7ba

See cosmos/cosmos-sdk#8396 (comment) Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

daeMOn63 pushed a commit to fetchai/cosmos-sdk that referenced this pull request Mar 1, 2022

store/multistore: revert a height limit increase from #8396 (#8466)

90d27d1

See cosmos/cosmos-sdk#8396 (comment) Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compatibility with the ARM architecture #8396

Compatibility with the ARM architecture #8396

RiccardoM commented Jan 20, 2021 •

edited

Loading

alessio commented Jan 20, 2021

RiccardoM commented Jan 20, 2021

alessio commented Jan 20, 2021

RiccardoM commented Jan 20, 2021

codecov bot commented Jan 27, 2021 •

edited

Loading

odeke-em left a comment

odeke-em Jan 27, 2021

RiccardoM Jan 28, 2021 •

edited

Loading

alessio Jan 28, 2021

Compatibility with the ARM architecture #8396

Compatibility with the ARM architecture #8396

Conversation

RiccardoM commented Jan 20, 2021 • edited Loading

Description

alessio commented Jan 20, 2021

RiccardoM commented Jan 20, 2021

alessio commented Jan 20, 2021

RiccardoM commented Jan 20, 2021

codecov bot commented Jan 27, 2021 • edited Loading

Codecov Report

odeke-em left a comment

Choose a reason for hiding this comment

odeke-em Jan 27, 2021

Choose a reason for hiding this comment

RiccardoM Jan 28, 2021 • edited Loading

Choose a reason for hiding this comment

alessio Jan 28, 2021

Choose a reason for hiding this comment

RiccardoM commented Jan 20, 2021 •

edited

Loading

codecov bot commented Jan 27, 2021 •

edited

Loading

RiccardoM Jan 28, 2021 •

edited

Loading