Box value encoded in a variant to reduce enum stack space

[RCasatta]

before

print-type-size type: `util::psbt::error::Error`: 120 bytes, alignment: 8 bytes
print-type-size     discriminant: 1 bytes
print-type-size     variant `CombineInconsistentKeySources`: 115 bytes
print-type-size         padding: 3 bytes
print-type-size         field `.0`: 112 bytes, alignment: 4 bytes
print-type-size     variant `InvalidKey`: 39 bytes
print-type-size         padding: 7 bytes
print-type-size         field `.0`: 32 bytes, alignment: 8 bytes

after

print-type-size type: `util::psbt::error::Error`: 40 bytes, alignment: 8 bytes
print-type-size     discriminant: 1 bytes
print-type-size     variant `InvalidKey`: 39 bytes
print-type-size         padding: 7 bytes
print-type-size         field `.0`: 32 bytes, alignment: 8 bytes
print-type-size     variant `DuplicateKey`: 39 bytes
print-type-size         padding: 7 bytes
print-type-size         field `.0`: 32 bytes, alignment: 8 bytes

util::psbt::error::Error is wrapped also in consensus::encode::Error and stack savings are gained there also

[apoelstra]

ACK 9906cea

[tcharding]

Interesting PR, I rarely think about the stack space of error enums. To educate me, can you please explain a few things if/when you have the time:

• What led you to do this?
• Why stop at the 115 byte variant, why not box all variants below X bytes?
• What made you do this enum in particular? Should we be doing this by convention for all the error enums codebase wide?

No rush, just if you have time. Thanks

[tcharding]

ACK 9906cea

[TheBlueMatt]

What made you do this enum in particular? Should we be doing this by convention for all the error enums codebase wide?

In general, Boxing is something that should be avoided - it adds some overhead by waiting on malloc+free and done too much leads to heap fragmentation and can substantially increase total application memory usage even after objects are free'd. However, this is an error variant - hopefully during normal operation we never hit this code anyway. In that case, by reducing the size of the error, we may be able to reduce the size of the ultimate Result<Success, Err> as long as Success is smaller than the whole Err. Its not exactly a critical thing for a few hundred bytes, but if we have a lot of those results, then it could add up.

[tcharding]

I see, paraphrasing to make sure I get you, the Result<T, E> has the same size as the biggest of T and E so even in the success path the size of the error effects stack usage.

So if we want to be particularly careful with stack usage we'd have to check the relation between T and E everywhere - sounds like hard work. Assuming the error path is infrequent then it would seem like good practice to box all error variants above some reasonable threshold, just to be proactive?

And we may want to be careful with stack usage as we endevour to support smaller devices (WASM, no-std, ect.)?

[TheBlueMatt]

And we may want to be careful with stack usage as we endevour to support smaller devices (WASM, no-std, ect.)?

I don't think WASM qualifies as a "smaller device" here, more tiny embedded things. I think we don't need to be universal about this, only parts of the library that are usable on embedded devices. e.g. no embedded (memory < 1 MiB) device is going to decode a block, as the whole thing is bigger than the embedded device's memory anyway, same goes for P2P messages, etc. For PSBT and signing transactions, on the other hand, this is the exact code that an embedded device may care about.

[tcharding]

Cool, so any error path that can be hit from PSBT code, that's a non-trivial amount of work to find out, right? Follow every type used in the psbt module and check its implementations. How serious are we about folk writing embedded signing devices in Rust? Is anyone doing this now that we know of?

[TheBlueMatt]

How serious are we about folk writing embedded signing devices in Rust? Is anyone doing this now that we know of?

Yea, actually no idea - is there a concrete use-case where this is important @RCasatta?

[sanket1729]

@RCasatta, I am curious how do you print byte alignments?

[sanket1729]

utACK 9906cea

[RCasatta]

@RCasatta, I am curious how do you print byte alignments?

RUSTFLAGS=-Zprint-type-sizes cargo +nightly build --release

Yea, actually no idea - is there a concrete use-case where this is important @RCasatta?

No concrete use-case in this case for me, I just spotted this big variant by looking at the aforementioned program. There are some examples linked from this page https://nnethercote.github.io/perf-book/type-sizes.html?highlight=print#measuring-type-sizes where changes like this make performance impact

[apoelstra]

Specter Wallet has indicated a desire to use rust-miniscript (and therefore rust-bitcoin) in embedded contexts. I take this pretty seriously. Maybe we have no actual users, but I think that's probably because rust-bitcoin can't be used rather than that it isn't used. So anything that improves the viability of rust-bitcoin-on-embedded is good by me.

Regarding the reasoning for this, Matt's summary sounds great to me. (a) generally we should try not to have big error enums, (b) PSBT is both non-speed-critical and likely to be used on a hardware wallet, so let's try to be nice with stack space.