Add a test with a 3Mb ordinals transaction

[sstone]

This tx highlighted a bug in how we encoded script size.

[t-bast]

Damn, we should really have exhaustive tests on the varint encoding, like we have in eclair (where we unit test every boundary). This is already implemented in lightning-kmp with the following function, which I find slightly more clear, what do you think?

private fun writeBigSize(input: ULong, out: Output) {
        when {
            input < 0xfdu -> writeByte(input.toInt(), out)
            input < 0x10000u -> {
                out.write(0xfd)
                writeU16(input.toInt(), out)
            }
            input < 0x100000000u -> {
                out.write(0xfe)
                writeU32(input.toInt(), out)
            }
            else -> {
                out.write(0xff)
                writeU64(input.toLong(), out)
            }
        }
    }

But I think there's a difference between lightning and bitcoin here, we don't use the same endianness, do we?

[sstone]

Yes bitcoin is little endian and LN is big endian. With the proposed changes we match what bitcoin core does which is always a good thing :)

[t-bast]

I find it a bit confusing that the implementation uses < for the first case but <= for the others to be honest...but if that matches the bitcoind implementation why not

[remyers]

For reference, Bitcoin Core confusingly calls these "compact size unsigned integers" and also sometimes varints, but CVarint is something different. I think the lightning-kmp implementation is clearer, though core names the function WriteCompactSize, so both could lead to naming confusion when comparing to core.

[t-bast]

LGTM!

[remyers]

I compared to the core code and tested with and without the fixes to double check the tests catch any issues. I only had a question and found an issue with the test.

[remyers]

For reference, Bitcoin Core confusingly calls these "compact size unsigned integers" and also sometimes varints, but CVarint is something different. I think the lightning-kmp implementation is clearer, though core names the function WriteCompactSize, so both could lead to naming confusion when comparing to core.

[remyers]

Looks good!

small nit:

val v = (1uL shl i) uses lower case uL, a few lines down it's an uppercase UL and a few lines above it's uL. Both are valid, Kotlin docs say "uL and UL explicitly tag literal as unsigned long"

[sstone]

small nit:

val v = (1uL shl i) uses lower case uL, a few lines down it's an uppercase UL and a few lines above it's uL. Both are valid, Kotlin docs say "uL and UL explicitly tag literal as unsigned long"

Ah now I have to fix :)