Conformance to exported symbols

[stickies-v]

I came across a couple of symbols that don't seem to be exported in bitcoinkernel.h (at least not in the current master branch):

• VERIFY_ALL_PRE_TAPROOT which is not a member of btck_ScriptVerificationFlags
• ScriptVerify.SpentOutputsMismatch and a couple of others which are not members of btck_ScriptVerifyStatus

This forces language bindings to implement business logic in the wrapper layer just to be spec compliant. I think any such cases should be left out of the test spec until they are exported in bitcoinkernel.h on master.

In the same vein, I think we should strive for the test spec to following btck naming conventions as much as possible. For example:

• btck_ScriptVerificationFlags has a WITNESS member, but it is called VERIFY_WITNESS in kernel-bindings-tests
• btck exports a btck_ScriptVerifyStatus enum, but it is called ScriptVerify in kernel-bindings-tests
• as already suggested on IRC, aligning method naming with btck functions would make sense

[stringintech]

Thanks a lot for the feedback @stickies-v. You're making fair points.

Re: extra error variants (TxInputIndex, SpentOutputsMismatch, ...) introduced in script_verify_errors.json: Personally, I find the extra guards included in the verify() implementation in the Rust wrapper nice; though I understand it doesn't make much sense to enforce them here when libbitcoinkernel hasn't included them.

1. Error structure refactoring

It seems I have to refactor the expected error structure. Right now we have:

"expected": {
  "error": {
    "type": "",
    "variant": ""
  }
}

But that doesn't seem suitable anymore if we want to stick to the btck symbols. Looking at bitcoinkernel.h, operations currently could return a non-zero value or nullptr indicating error (except for btck_script_pubkey_verify() which returns zero on failure). I'm thinking about how these cases could be unified and presented in the JSON structure; perhaps the following for when we don't have the reason for the failure:

"expected": {
  "error": {}
}

And when we have the reason:

"expected": {
  "error": {
    "reason": "btck_ScriptVerifyStatus_ERROR_INVALID_FLAGS_COMBINATION"
  }
}

2. VERIFY_ALL_PRE_TAPROOT representation

Would it make more sense to you if it's replaced with an array of flags that should be ORed together:

"flags": [
  "btck_ScriptVerificationFlags_P2SH",
  "btck_ScriptVerificationFlags_DERSIG",
  "btck_ScriptVerificationFlags_NULLDUMMY",
  "btck_ScriptVerificationFlags_CHECKLOCKTIMEVERIFY",
  "btck_ScriptVerificationFlags_CHECKSEQUENCEVERIFY",
  "btck_ScriptVerificationFlags_WITNESS"
]

Or just use the numerical result value?

---

I'd appreciate your further feedback on the above two points.

cc @janb84 @yuvicc

[stickies-v]

Personally, I find the extra guards included in the verify() implementation in the Rust wrapper nice

I agree, and if we find that every language bindings wants to implement them in the wrapper logic I think it might be worth discussing to just add them to the C header.

I'm thinking about how these cases could be unified and presented in the JSON structure; perhaps the following for when we don't have the reason for the failure:

Since errors can be null, I think we need an explicit success field (as you currently do), I think "error": {} should be equivalent to the json object not having an error key at all. I think the exact return type of btck (false, 0, nullptr) on failure can all be generalized into "succes": "false". The (optional) "error" field can the encode information about the specifics of the error, if available. Ideally, it is structured so that clients can automatically parse and convert it into their local naming convention. I think the below makes that quite easy:

{
  "success": "false",
  "error": {
    "code": {
      "type": "btck_ScriptVerifyStatus",
      "member": "ERROR_INVALID_FLAGS_COMBINATION"
    }
  }
}

Would it make more sense to you if it's replaced with an array of flags that should be ORed together: ... Or just use the numerical result value?

Either approach seems acceptable to me. Using numerical result values seems more elegant, but I think there is value in testing the flag names since that's what users will actually be using, so that probably has my preference (even though I don't like that the OR operations are implied but not specified)

[stickies-v]

I think it might be worth discussing to just add them to the C header.

I've opened bitcoin/bitcoin#33943 to address this. It doesn't have status code for malformed requests - I'm not convinced that is meaningful, but the approach does allow kernel-bindings-tests to properly test that these malformed edge cases are handled uniformly across language bindings. Hope to hear your thoughts on the PR!

[janb84]

I think it might be worth discussing to just add them to the C header.

I've opened bitcoin/bitcoin#33943 to address this. It doesn't have status code for malformed requests - I'm not convinced that is meaningful, but the approach does allow kernel-bindings-tests to properly test that these malformed edge cases are handled uniformly across language bindings. Hope to hear your thoughts on the PR!

@stickies-v Thank you for opening the PR, I will have a look soon. I had the same struggle, I found the enum extension of the rust impl. logical and nice to have but it made the implementation kinda not optimal. This will solve that :)

Since errors can be null, I think we need an explicit success field (as you currently do), [...] quite easy:

I agree with the proposal of the addition of succes field , it can even represent the nullptr state:

{
  "success": "false",
  "error": {}
}

Would it make more sense to you if it's replaced with an array of flags that should be ORed together: ... Or just use the numerical result value?

Both would make sense to me, if both are to be expected.

[stringintech]

I have addressed the feedback and suggestions in #7.

Thank you!

[yuvicc]

I tend to agree with the approach mentioned by @stickies-v.

{
"success": "false",
"error": {
"code": {
"type": "btck_ScriptVerifyStatus",
"member": "ERROR_INVALID_FLAGS_COMBINATION"
}
}
}

Would it make more sense to you if it's replaced with an array of flags that should be ORed together: ... Or just use the numerical result value?

Either approach seems acceptable to me. Using numerical result values seems more elegant, but I think there is value in testing the flag names since that's what users will actually be using, so that probably has my preference (even though I don't like that the OR operations are implied but not specified)

[stringintech]

I am thinking whether we should reconsider bringing back the test cases I removed in #7 (error_tx_input_index_out_of_bounds, error_spent_outputs_mismatch, error_invalid_flags) as they seem to be valuable in validating that binding implementations perform input validation before calling into the C API and prevent crashes resulting from a naive implementation. Not sure how it could fit in the new structure; maybe a generic failure with no error code?

{
  "id": "error_tx_input_index_out_of_bounds",
  "description": "Binding must validate input_index before calling kernel",
  "expected": {
    "success": false
  }
}

What do you think? An alternative is to just wait to see how kernel is improved in dealing with bad user input.

[stickies-v]

as they seem to be valuable in validating that binding implementations perform input validation before calling into the C API and prevent crashes resulting from a naive implementation.

I agree that it is useful, and I think we should work towards being able to do that, but I think doing it now will cause more problems than benefit

What do you think? An alternative is to just wait to see how kernel is improved in dealing with bad user input.

If you add them back now, I don't think you'll be able to make the tests succeed on the C API itself, and that seems like a big problem to me. I really don't think it makes sense to have tests that violate the API's preconditions.

[stringintech]

I really don't think it makes sense to have tests that violate the API's preconditions.

Can you elaborate a bit on this please?
I understand it is challenging to have such tests if the goal is to stick to btck symbols, but it does not sound unreasonable to have them if at some point we decide to leave the C API minimal and expect bindings to implement additional validation layers.

I agree to delay adding them though, hoping for the C API to make it less challenging.

[stickies-v]

if at some point we decide to leave the C API minimal and expect bindings to implement additional validation layers.

That is exactly what I think shouldn't happen. Bindings should make the interface idiomatic, nothing else. They shouldn't be relied on for extra validation, because that risks diverging behaviour across users depending on their language choice. It creates first-class and second-class citizens, whereas the C API and all of its language bindings should be on equal footing.

In this particular case, the C API already does the validation, it just makes it untestable because it asserts these preconditions. If we can't test that behaviour is identical across implementations, I think it shouldn't be part of the interface. For that reason, I think it would be useful if someone built a handler for the C API.

[stringintech]

because that risks diverging behaviour across users depending on their language choice.

That clarifies it for me. Thanks.

I think it would be useful if someone built a handler for the C API.

That would be interesting... Would like to look into that when I get some more time.