DeterministicKey: ensure depth < 256. depth is serialized as uint8

[afk11]

Since the depth serialization is a single byte deriving more than 255 levels is causing problems. It's not explicitly limited to 255 in the BIP, but this check seems worth it to rule out mistakes or denial of service

[schildbach]

Actually the BIP does state "1 byte": https://github.com/bitcoin/bips/blob/master/bip-0032.mediawiki#Key_identifiers

In code, we simply cast it to Java "byte", which is signed. I wonder if this already fails with values >127.

I wonder if there is a test vector availble for such edge cases.

[afk11]

@schildbach

In code, we simply cast it to Java "byte", which is signed. I wonder if this already fails with values >127.

Ah, good question. I didn't check that I'll be honest.

I wonder if there is a test vector availble for such edge cases.

Not in the BIP. Sometimes such bugs get discovered and test cases shared, but they don't get the attention they need really. (bitcore + btcd both had a bug in bip32 where if a 32byte ByteArrays left most bytes were 0x00 it'd accidentally trim to 31.. so a non-portable wallet)

I think a community project that defined a testing interface, and contains test fixtures.. if the long running bitcoin libraries implemented the interface, each time a new fixture is added they could all be checked. A simple start might just be BIP39/BIP32. Minimal interface to test, but covers a lot of operations where things can go wrong that we hope they'd detect.

[afk11]

Amended previous commit. Thanks for the review and sorry to keep you waiting