wally_tx_to_bytes: don't return WALLY_OK if buffer is too short.

[rustyrussell]

WALLY_ENOMEM is the most suitable (since WALLY_EINVAL can happen for many other reasons), but document it.

Having the call "succeed" without actually doing anything is very dangerous.

Signed-off-by: Rusty Russell rusty@rustcorp.com.au

[glslang]

I don't necessarily disagree but a few comments (this was a topic of discussion before),

1. this is a pattern in wally so this change is potentially insufficient if the intention is to fix the pattern
2. this is a breaking API change which will cause client code to change potentially so it should also be documented in CHANGES.md
3. WALLY_ENOMEM appears incorrect since no allocation (or attempt thereof) has happened.
4. consider adding a new error code so clients know to check the written output param

[rustyrussell]

1. This may be an internal pattern, but it is too dangerous to expose. Fixing internals is another step, I think.
2. Good idea.
3. Agreed, but these routines could not previously return WALLY_ENOMEM (though it's the caller, not the environment which has provided insufficient resources).
4. There is also an API risk of adding another WALLY error code. It is not documented which routines can return which errors: should all callers handle the new WALLY_ENOSPC error? This would require much more auditing of not only libwally, but projects which use it.

The alternative is to return WALLY_EINVAL, but set the written_len to 0 if it's really EINVAL, and use a non-zero number to indicate we need more space. A but more cumbersome for callers, perhaps, but avoids the issue of a new error code.

[glslang]

It's an external pattern. Used in src/aes.c and src/script.c so it's exposed in many other places.
EINVAL seems better to me. ENOMEM really is allocation has failed. This case is different. Yes, there's lack of a resource but I can retry it whilst with ENOMEM things are fuzzier in that respect.
There's no real need to document the error codes because is EINVAL for an wrong argument passed to a function, ERROR for something has failed, ENOMEM couldn't allocate. wally_core.h documents this.

[rustyrussell]

It's an external pattern. Used in src/aes.c and src/script.c so it's exposed in many other places.

Sorry, they all need to be fixed where they are exposed through the public API. This is an undocumented, and deeply surprising feature which will lead to bugs.

There's no real need to document the error codes because is EINVAL for an wrong argument passed to a function, ERROR for something has failed, ENOMEM couldn't allocate. wally_core.h documents this.

This, again, is wrong.

libwally's implementation is currently inefficient, and performs multiple passes to marshal and unmarshal things, using code duplication. An efficient implementation would allow the caller to allocate a buffer, try to marshal / unmarshal, then enlarge as necessary. We should prepare for that.

But libwally has quirky, undocumented and untested requirements! This means it can return WALLY_EINVAL on things which (to the caller) will seem perfectly valid. For example, in some places a zero length argument is not allowed if the matching pointer is non-NULL.

So the caller really needs to know: is this EINVAL because I have a bug, or is it just the buffer needs to be bigger? And so it needs to be documented.

[glslang]

Sorry, they all need to be fixed where they are exposed through the public API. This is an undocumented, and deeply surprising feature which will lead to bugs.

This is what I alluded to in my original comments. No point in just fixing it here.

This, again, is wrong.

No. You're in disagreement but was clearly a design decision from the beginning.

libwally's implementation is currently inefficient, and performs multiple passes to marshal and unmarshal things, using code duplication. An efficient implementation would allow the caller to allocate a buffer, try to marshal / unmarshal, then enlarge as necessary. We should prepare for that.

This an orthogonal argument to what was being discussed. But wally needs an official maintainer or it'll simply rely on piecewise contributions that won't fully address its shortcomings. I'm just giving historical context and gently pushing for these PRs to be far more committed than they currently are :).

But libwally has quirky, undocumented and untested requirements! This means it can return WALLY_EINVAL on things which (to the caller) will seem perfectly valid. For example, in some places a zero length argument is not allowed if the matching pointer is non-NULL.

This is only the case where wally will unconditionally de-reference that pointer. Not true on the sha256/sha512 functions.

So the caller really needs to know: is this EINVAL because I have a bug, or is it just the buffer needs to be bigger? And so it needs to be documented.

Yes it does. But documentation won't help you. Any maintenance programmer will tell you that documentation only goes so far. It's the headers and the code and being able to navigate it that really makes the difference. Hence returning a new error code. It's unambiguous.

[rustyrussell]

Sorry, they all need to be fixed where they are exposed through the public API. This is an undocumented, and deeply surprising feature which will lead to bugs.

This is what I alluded to in my original comments. No point in just fixing it here.

This, again, is wrong.

No. You're in disagreement but was clearly a design decision from the beginning.

Because it's wrong :)

libwally's implementation is currently inefficient, and performs multiple passes to marshal and unmarshal things, using code duplication. An efficient implementation would allow the caller to allocate a buffer, try to marshal / unmarshal, then enlarge as necessary. We should prepare for that.

This an orthogonal argument to what was being discussed. But wally needs an official maintainer or it'll simply rely on piecewise contributions that won't fully address its shortcomings. I'm just giving historical context and gently pushing for these PRs to be far more committed than they currently are :).

Sorry, I was unclear. It's the same argument: if libwally was optimally efficient, it would make sense for the caller to try to marshal into the buffer, and loop if it failed. Today, libwally just marshals in two passes, so there's no real performance benefit from doing this.

But libwally has quirky, undocumented and untested requirements! This means it can return WALLY_EINVAL on things which (to the caller) will seem perfectly valid. For example, in some places a zero length argument is not allowed if the matching pointer is non-NULL.

This is only the case where wally will unconditionally de-reference that pointer. Not true on the sha256/sha512 functions.

No, I discovered this in wally_tx_input_init_alloc()'s "script" argument. If script_len is 0, script must be NULL, otherwise it returns WALLY_EINVAL :(

See also #192

So the caller really needs to know: is this EINVAL because I have a bug, or is it just the buffer needs to be bigger? And so it needs to be documented.

Yes it does. But documentation won't help you. Any maintenance programmer will tell you that documentation only goes so far. It's the headers and the code and being able to navigate it that really makes the difference. Hence returning a new error code. It's unambiguous.

OK, WALLY_ENOSPC? Or a more explicit WALLY_BUFSHORT? Naming is hard!

[jgriffiths]

The behaviour of functions returning variable output buffers has been documented and this is now linked to automatically in every function that returns one. The behaviour is that the function returns WALLY_OK and writes the required length. the caller must check the returned length fits within their buffer.

The initial reason was for SWIG wrapping: Returning an error code through the swig wrappers throws an exception in the generated code and loses the required length information. We currently have over 230 functions to expose through C, C++, python, python-cffi, Java and Javascript, and in many cases the wrappers have not been kept in sync with library changes; these need to be exposed (and tested in some cases) as a matter of urgency. We also need better docs and examples which will hopefully be coming if we have time.

If we decide to change this in the future we will need to change all 34 functions that are affected at once and announce it in CHANGES.md. Having a mix of different strategies per-function is even more dangerous than consistently using a single one.

I'm closing this PR as it relates to the tx call, feel free to raise an issue to address this differently in a future library release.