paulmillr / scure-base

Secure, audited & 0-deps implementation of bech32, base64, base32, base16 & base58
https://paulmillr.com/noble/#scure
MIT License
106 stars 13 forks source link

Tests for `convertRadix` and `convertRadix2` #25

Open mahnunchik opened 9 months ago

mahnunchik commented 9 months ago

I've used convertRadix function on the cashaddr implementation https://github.com/paulmillr/scure-base/pull/24 and it converts 5-bit number correctly according to the checksum tests.

But I've faced with that simple test gives incorrect result:

const data = [1];

console.log(convertRadix2(data, 8, 5, true));
// [ 0, 4 ] - correct
console.log(convertRadix(data, 2 ** 8, 2 ** 5));
// [ 1 ] - incorrect

I thought that I could understand the behavior from tests, but there are no tests for these methods.

Is this my mistake in use or is the method not implemented correctly?

paulmillr commented 9 months ago

convertRadix and convertRadix2 are different algorithms:

It is possible to implement convertRadix2 on bigints to use same method in bech32 and cashaddr but according to your comment it is 5x times slower.Should it be tested again in modern browsers ?

BigInts are still very slow, 5x slower for convertRadix, convertRadix2 will be even slower.

You can implement convertRadix2Big if you need bigger numbers. Or convertRadix2Int, using modulo division instead of masks, it will be slower than current implementation, but will allow numbers up to 2**53

paulmillr commented 8 months ago

should be clarified in readme and code tho

mahnunchik commented 8 months ago

I'm completely confused the implementations convertRadix, convertRadix2, radix, and radix2.

Using radix and radix2 gives the same result:

const data = [1];

console.log(utils.radix(2 ** 5).encode(new Uint8Array(data)))
// [ 1 ]
console.log(utils.radix2(5).encode(new Uint8Array(data)))
// [ 0, 4 ]

Is it the issue or desired behavior?

I think implementation logic should be clarified and/or tests added. If the behavior is different maybe there is sense to rename methods?

paulmillr commented 8 months ago

radix has no padding, radix2 has padding. that's main diff.

paulmillr commented 8 months ago
const { convertRadix, convertRadix2, radix, radix2 } = require('./lib/index.js');

// > But I've faced with that simple test gives incorrect result:

// No, this is actually correct result, 1 % 2**8 === 1 % 2**5
console.log(convertRadix([1], 2 ** 8, 2 ** 5)); // -> [ 1 ]
// If it element is bigger or equal than 2**5, it will overflow to next element
console.log(convertRadix([2 ** 5], 2 ** 8, 2 ** 5)); // [ 1, 0 ]

// Same happens with convertRadix2:
// NOTE: since u8 = 2 u4, it will always return two elements for single one
console.log(convertRadix2([1], 8, 4, false)); // [ 0, 1 ]
console.log(convertRadix([1], 2 ** 8, 2 ** 4)); // -> [ 1 ] (can be represented as 1 elm)
// If bigger -> will overflow to next element
console.log(convertRadix2([2 ** 4], 8, 4, false)); // [ 1, 0 ]
console.log(convertRadix([2 ** 4], 2 ** 8, 2 ** 4)); // -> [ 1, 0 ] (same!)

// What happens with 2**8 -> 2**5?
// Since we cannot represent 1 8bit number as multiple 5bit number, we need to add padding
console.log(convertRadix2([0, 0, 0, 0, 1], 8, 5)); // [ 0, 0, 0, 0, 0, 0, 0, 1 ]

// Otherwise there will be error:
console.log(convertRadix2([1], 8, 5, true)); // [ 0, 4 ]
// Reverse. NOTE: no padding here, because we add it on encoding
console.log(convertRadix2([0, 4], 5, 8)); // [ 1 ]
mahnunchik commented 8 months ago

I understand. But it is really hard to understand why this is correct. I think at least a mention in the readme is necessary.