Adding message length to the callbacks #639

pull elichai wants to merge 3 commits into bitcoin-core:master from elichai:callback changing 4 files +17 −12
  1. elichai commented at 2:51 pm on June 13, 2019: contributor

    This will help a lot for downstream code that doesn’t have easy access to libc(strlen etc.)

    Any thoughts about this?

    cc downstream https://github.com/rust-bitcoin/rust-secp256k1/pull/115

    (I wasn’t sure when is CHECK vs VERIFY_CHECK is used so would appreciate a feedback if I used it right)

  2. Added a message len to the callbacks in the headers 916f4d6362
  3. Added a message len to the callbacks implementation fd23b2f57b
  4. Updated the tests with the new callbacks function signature (msg len) abe9d5ac9c
  5. real-or-random commented at 3:12 pm on June 13, 2019: contributor

    Ah, I considered this too in rust-bitcoin/rust-secp256k1#115. :)

    The problem with this approach is that

    1. It adds a dependency to strlen(). I think with all our recent efforts for supporting embedded systems, this part of the PR is rather a step back.
    2. It breaks the callback API, see #595 on discussions about whether it is okay to break this or not.

    The first point is easily solved by implementing strlen on our own, it’s just three lines. The second point is difficult.

    My take is rather that downstream should implement strlen if it requires it. It’s also just three lines there.

    edit: Another thing we could do here is to guarantee a fixed minimum message size. This will be somewhat arbitrary but could be useful. Then the user knows “it’s okay to print exactly X bytes” (and maybe I don’t print the full message then but I’m not doing anything wrong).

  6. elichai commented at 3:17 pm on June 13, 2019: contributor

    hmm shouldn’t embedded systems has some sort of a libc? Altough I can see why people would want to compile to embedded without linking to anything.

    if we need to implement that anyway then I guess downstream could implement this as easily as us.

    I’ll leave this PR open for a while see what other people think about this.

    Regarding breaking the API I do agree with #595 (comment) that if the change is good then it’s mostly fine (these callbacks should almost never be called and most people won’t provide their own callbacks)

  7. real-or-random commented at 3:23 pm on June 13, 2019: contributor

    Unless someone else will have a different opinion until tomorrow i’ll close this PR.

    Yeah or maybe wait somewhat longer, my single opinion is not authoritative of course (nor is anyone else’s.)

  8. elichai cross-referenced this on Jun 13, 2019 from issue Updating secp256k1 and linking with external callbacks by elichai
  9. elichai closed this on Jun 25, 2019


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/secp256k1. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-22 06:15 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me