Skip to content

Fix: ln calculation determinism#789

Merged
amimart merged 8 commits intodevfrom
arnaud/engn-3577-fix-ln-determinism
Apr 3, 2025
Merged

Fix: ln calculation determinism#789
amimart merged 8 commits intodevfrom
arnaud/engn-3577-fix-ln-determinism

Conversation

@amimart
Copy link
Contributor

@amimart amimart commented Apr 2, 2025

Purpose of Changes and their Description

This fixes the natural logarithmic calculation non-deterministic behaviour by switching from apd's implementation to a custom one.

For instance with the following snippet using apd:

var dec128Context = &apd.Context{
	Precision:   34,
	MaxExponent: apd.MaxExponent,
	MinExponent: apd.MinExponent,
	Traps:       apd.DefaultTraps,
	Rounding:    apd.RoundDown,
}
i, _, _ := apd.NewFromString("1.6285091944505809264504560045920167")
var r apd.Decimal
dec128Context.Ln(&r, i)
fmt.Println(r.String())

It outputs to 0.4876649916811116824516548471782886 on arm64 and 0.4876649916811116824516548471782887 on amd64, which can cause app hash mismatch.

The root cause is that apd in some cases uses a first approximation with the standard math.Log() which is platform dependent, I've raised an issue on apd.

To solve the issue I've followed a similar strategy to Osmosis by implementing the binary logarithm algorithm described here, with some adaptations to make it work with apd.Decimal. The natural logarithm is then derived from it.

The precision seems to slightly differ from apd's implementation but in the order of 1ulp on non close to limits values.

I've added some benchmark, and here's the benchmark results comparing with old apd implementation:

goos: darwin
goarch: arm64
pkg: github.com/allora-network/allora-chain/math
cpu: Apple M3 Pro
BenchmarkLnAPD
BenchmarkLnAPD-12    	    2455	    423923 ns/op	  369017 B/op	    7118 allocs/op
BenchmarkLn
BenchmarkLn-12       	    1916	    604703 ns/op	  593910 B/op	   11306 allocs/op
PASS

It is much slower with more allocations, this seems not to be used intensively in the codebase so I'd tend to consider it ok. There's some perf improvements that can be made using different approaches of ln calculation though.

Are these changes tested and documented?

  • If tested, please describe how. If not, why tests are not needed.
  • If documented, please describe where. If not, describe why docs are not needed.
  • Added to Unreleased section of CHANGELOG.md?

amimart added 3 commits April 2, 2025 01:58
The value used in the test is subject to discrepancies using apd Ln
implementation between amd64 and arm64
@amimart amimart self-assigned this Apr 2, 2025
@amimart amimart force-pushed the arnaud/engn-3577-fix-ln-determinism branch from 5ae2790 to 7e44ce9 Compare April 2, 2025 00:51
@amimart amimart marked this pull request as ready for review April 2, 2025 00:51
@amimart amimart force-pushed the arnaud/engn-3577-fix-ln-determinism branch from 9629afd to 0c2038e Compare April 2, 2025 15:29
@amimart amimart merged commit 399f9fb into dev Apr 3, 2025
9 checks passed
@amimart amimart deleted the arnaud/engn-3577-fix-ln-determinism branch April 3, 2025 23:17
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.

3 participants