Default Display of ChildNumber changed with 0.26.1

[afilini]

The default Display fmt of ChildNumber was changed in this commit 017cd71 and it's causing test failures in BDK because the descriptors we print have a different format.

Originally hardened ChildNumbers were always printed with ', but now that's only done when the "alternate" form is explicitly enabled, which means that just printing a path with the standard {} results in a different string compared to the previous releases.

[RCasatta]

Even if I a prefer the h over the apex I think we should stick to the apex as default, for example, if you give core a descriptor with h to getdescriptorinfo the returned descriptor contain the apex

[afilini]

Considering that the descriptor checksum is computed on the string I wonder if this breaks checksum compatibility with Core in rust-miniscript

[afilini]

Yup, cargo test on rust-bitcoin/rust-miniscript@f550b9b fails and the checksums are different

failures:

---- descriptor::key::test::test_wildcard stdout ----
thread 'descriptor::key::test::test_wildcard' panicked at 'assertion failed: `(left == right)`
  left: `"m/0h/1h/2"`,
 right: `"m/0\'/1\'/2"`', src/descriptor/key.rs:775:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- descriptor::tests::parse_descriptor_key stdout ----
thread 'descriptor::tests::parse_descriptor_key' panicked at 'assertion failed: `(left == right)`
  left: `"[78412e3a/44h/0h/0h]xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL/1/*"`,
 right: `"[78412e3a/44\'/0\'/0\']xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL/1/*"`', src/descriptor/mod.rs:1226:9

---- descriptor::key::test::test_deriv_on_xprv stdout ----
thread 'descriptor::key::test::test_deriv_on_xprv' panicked at 'assertion failed: `(left == right)`
  left: `"[2cbe2a6d/0h/1h]tpubDBrgjcxBxnXyL575sHdkpKohWu5qHKoQ7TJXKNrYznh5fVEGBv89hA8ENW7A8MFVpFUSvgLqc4Nj1WZcpePX6rrxviVtPowvMuGF5rdT2Vi/2"`,
 right: `"[2cbe2a6d/0\'/1\']tpubDBrgjcxBxnXyL575sHdkpKohWu5qHKoQ7TJXKNrYznh5fVEGBv89hA8ENW7A8MFVpFUSvgLqc4Nj1WZcpePX6rrxviVtPowvMuGF5rdT2Vi/2"`', src/descriptor/key.rs:795:9

---- descriptor::tests::parse_with_secrets stdout ----
thread 'descriptor::tests::parse_with_secrets' panicked at 'assertion failed: `(left == right)`
  left: `"wpkh([a12b02f4/44\'/0\'/0\']xpub6BzhLAQUDcBUfHRQHZxDF2AbcJqp4Kaeq6bzJpXrjrWuK26ymTFwkEFbxPra2bJ7yeZKbDjfDeFwxe93JMqpo5SsPJH6dZdvV9kMzJkAZ69/0/*)#u37l7u8u"`,
 right: `"wpkh([a12b02f4/44h/0h/0h]xpub6BzhLAQUDcBUfHRQHZxDF2AbcJqp4Kaeq6bzJpXrjrWuK26ymTFwkEFbxPra2bJ7yeZKbDjfDeFwxe93JMqpo5SsPJH6dZdvV9kMzJkAZ69/0/*)#0qpqyxsl"`', src/descriptor/mod.rs:1432:9

---- descriptor::tests::test_parse_descriptor stdout ----
thread 'descriptor::tests::test_parse_descriptor' panicked at 'assertion failed: `(left == right)`
  left: `"wpkh([2cbe2a6d/44h/0h/0h]tpubDCvNhURocXGZsLNqWcqD3syHTqPXrMSTwi8feKVwAcpi29oYKsDD3Vex7x2TDneKMVN23RbLprfxB69v94iYqdaYHsVz3kPR37NQXeqouVz/0/*)#qxjejldn"`,
 right: `"wpkh([2cbe2a6d/44\'/0\'/0\']tpubDCvNhURocXGZsLNqWcqD3syHTqPXrMSTwi8feKVwAcpi29oYKsDD3Vex7x2TDneKMVN23RbLprfxB69v94iYqdaYHsVz3kPR37NQXeqouVz/0/*)#nhdxg96s"`', src/descriptor/mod.rs:1369:9


failures:
    descriptor::key::test::test_deriv_on_xprv
    descriptor::key::test::test_wildcard
    descriptor::tests::parse_descriptor_key
    descriptor::tests::parse_with_secrets
    descriptor::tests::test_parse_descriptor

test result: FAILED. 50 passed; 5 failed; 0 ignored; 0 measured; 0 filtered out; finished in 2.03s

[dr-orlovsky]

Just for the history record, this was discussed and everyone in discussion agreed that this was acceptable for minor version change: #567 (comment)

If it breaks Bitcoin Core checksum compatibility this is still an issue I believe and should be fixed.

[sgeisler]

Damn it, that shows again: no matter how clever we feel about our reasons for breaking semver, we just shouldn't 😬

[afilini]

I'd argue that this kind of breaking change is even worse that straight up breaking the API in a patch release: at least API changes are caught at compile time, this one just breaks the code silently

[apoelstra]

Yeah :( it breaks my heart to revert this but if we break checksums at runtime then we definitley need to fix it.

Sorry guys.