RFC: libbitcoinconsensus #5235

pull theuni wants to merge 10 commits into bitcoin:master from theuni:libbitcoinconsensus changing 10 files +474 −16
  1. theuni commented at 6:56 am on November 7, 2014: member

    This includes #5212, which will need to be merged before this one.

    This adds a consensus lib that offers nothing more than transaction verification. It does not rely on any bitcoind globals or state, and boost is not required.

    The external api still needs some attention, it’s not intended to be final. What’s here is just something to get started, I believe @sipa had some plans for mapping external api flags to internal ones.

    Build-wise, I believe it’s feature complete other than Windows dll’s. They work fine, but libtool refuses to statically-link libc/libgcc. That can be fixed post-merge.

  2. sipa commented at 8:18 am on November 7, 2014: member 
    0./autogen.sh
1./configure --disable-wallet --without-gui
2...
3checking whether to build libraries... ./configure: line 23562: ],: command not found
4yes
5...
  3. theuni force-pushed on Nov 7, 2014
  4. theuni commented at 8:24 am on November 7, 2014: member
    Er, sorry for the busted c/p. Fixed.
  5. laanwj added this to the milestone 0.10.0 on Nov 7, 2014
  6. laanwj added the label Feature on Nov 7, 2014
  7. laanwj commented at 5:21 pm on November 17, 2014: member

    This needs some additions to .gitignore:

     0?? src/.libs/
 1?? src/core/libbitcoinconsensus_la-transaction.lo
 2?? src/crypto/libbitcoinconsensus_la-ripemd160.lo
 3?? src/crypto/libbitcoinconsensus_la-sha1.lo
 4?? src/crypto/libbitcoinconsensus_la-sha2.lo
 5?? src/libbitcoinconsensus.la
 6?? src/libbitcoinconsensus_la-eccryptoverify.lo
 7?? src/libbitcoinconsensus_la-ecwrapper.lo
 8?? src/libbitcoinconsensus_la-hash.lo
 9?? src/libbitcoinconsensus_la-pubkey.lo
10?? src/libbitcoinconsensus_la-uint256.lo
11?? src/libbitcoinconsensus_la-utilstrencodings.lo
12?? src/script/libbitcoinconsensus_la-bitcoinconsensus.lo
13?? src/script/libbitcoinconsensus_la-interpreter.lo
14?? src/script/libbitcoinconsensus_la-script.lo
  8. in src/script/bitcoinconsensus.h: in 08b93cad32 outdated 
    36+};
37+
38+/// Returns true if the input nIn of the serialized transaction pointed to by
39+/// txTo correctly spends the scriptPubKey pointed to by scriptPubKey under
40+/// the additional constraints specified by flags.
41+EXPORT_SYMBOL bool bitcoinconsensus_verify_script(const unsigned char *scriptPubKey, const unsigned int scriptPubKeyLen,
    laanwj commented at 5:41 pm on November 17, 2014:
    What is the reason for having const here on basic types like unsigned int?
    laanwj commented at 6:08 pm on November 17, 2014:
    Should we be using ‘bool’ here? That is C99. As we know, some platforms cough MSVC cough still don’t implement that, so it may be better to just return a 0/1 int.
    theuni commented at 6:13 pm on November 17, 2014:
    No reason, will change. ACK on not using bool as well.
  9. in src/script/bitcoinconsensus.cpp: in 08b93cad32 outdated 
    0@@ -0,0 +1,63 @@
1+#include "bitcoinconsensus.h"
    Diapolo commented at 6:24 pm on November 17, 2014:
    Missing license header.
  10. in src/script/bitcoinconsensus.h: in 08b93cad32 outdated 
    0@@ -0,0 +1,53 @@
1+#ifndef H_BITCOIN_BITCOINCONSENSUS
    Diapolo commented at 6:24 pm on November 17, 2014:
    Also missing license header. Nit: Also should be BITCOIN_SCRIPT_BITCOINCONSENSUS_H.
  11. in src/script/bitcoinconsensus.h: in 08b93cad32 outdated 
    37+
38+/// Returns true if the input nIn of the serialized transaction pointed to by
39+/// txTo correctly spends the scriptPubKey pointed to by scriptPubKey under
40+/// the additional constraints specified by flags.
41+EXPORT_SYMBOL bool bitcoinconsensus_verify_script(const unsigned char *scriptPubKey, const unsigned int scriptPubKeyLen,
42+                                    const unsigned char *txTo        , const unsigned int txToLen,
    Diapolo commented at 6:24 pm on November 17, 2014:
    Some weird indentation here?
  12. in src/script/script_error.h: in 08b93cad32 outdated 
    0@@ -0,0 +1,53 @@
1+// Copyright (c) 2009-2010 Satoshi Nakamoto
2+// Copyright (c) 2009-2014 The Bitcoin developers
3+// Distributed under the MIT software license, see the accompanying
4+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
5+
6+#ifndef BITCOIN_SCRIPT_TYPES_H
    Diapolo commented at 6:26 pm on November 17, 2014:
    Nit: If we use what we recently had as some form of policy this should read BITCON_SCRIPT_ERROR_H, right? Also the file should be named just error.h then? Just saying :)…
    theuni commented at 6:37 pm on November 17, 2014:
    @Diapolo This is going to be an installed header, so asking a 3rd-party application to #include <error.h> is a no-go. If anything, it should probably be renamed to bitcoinconsensus_error.h.
    makhmet commented at 6:47 pm on November 17, 2014:

    СПАСИБО!

    2014-11-17 21:37 GMT+03:00 Cory Fields notifications@github.com:

    In src/script/script_error.h:

    @@ -0,0 +1,53 @@ +// Copyright (c) 2009-2010 Satoshi Nakamoto +// Copyright (c) 2009-2014 The Bitcoin developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#ifndef BITCOIN_SCRIPT_TYPES_H

    @Diapolo https://github.com/Diapolo This is going to be an installed header, so asking a 3rd-party application to #include <error.h> is a no-go. If anything, it should probably be renamed to bitcoinconsensus_error.h.

    — Reply to this email directly or view it on GitHub https://github.com/bitcoin/bitcoin/pull/5235/files#r20454238.

    Diapolo commented at 6:48 pm on November 17, 2014:
    @theuni Alright, didn’t take that into consideration, when suggesting that :).
  13. laanwj commented at 7:49 pm on November 17, 2014: member

    Works for me.

    I patched main.cpp to write out script verification operations

     0diff --git a/src/main.cpp b/src/main.cpp
 1index 2bff781..80433d8 100644
 2--- a/src/main.cpp
 3+++ b/src/main.cpp
 4@@ -1359,8 +1359,16 @@ void UpdateCoins(const CTransaction& tx, CValidationState &state, CCoinsViewCach
 5
 6 bool CScriptCheck::operator()() const {
 7     const CScript &scriptSig = ptxTo->vin[nIn].scriptSig;
 8-    if (!VerifyScript(scriptSig, scriptPubKey, nFlags, CachingSignatureChecker(*ptxTo, nIn, cacheStore)))
 9+    bool rv;
10+    if (!(rv=VerifyScript(scriptSig, scriptPubKey, nFlags, CachingSignatureChecker(*ptxTo, nIn, cacheStore))))
11         return error("CScriptCheck() : %s:%d VerifySignature failed", ptxTo->GetHash().ToString(), nIn);
12+
13+    CDataStream ssTx(SER_NETWORK, PROTOCOL_VERSION);
14+    ssTx << *ptxTo;
15+    std::string strTxTo = HexStr(ssTx.begin(), ssTx.end());
16+    std::string strScriptPubKey = HexStr(scriptPubKey.begin(), scriptPubKey.end());
17+    printf("verify_script %s %s %i %i %i\n", strTxTo.c_str(), strScriptPubKey.c_str(), nIn, nFlags, rv);
18+
19     return true;
20 }

    Then, I wrote a Python script that uses ctypes to load and call into the library, verifying the operations written

     0from __future__ import print_function, division
 1import json, binascii, os
 2import ctypes
 3from ctypes import c_uint
 4
 5class BitcoinConsensus(object):
 6    def __init__(self, path):
 7        self.l = ctypes.CDLL(path)
 8
 9    def version(self):
10        return self.l.bitcoinconsensus_version()
11
12    def verify_script(self, scriptPubKey, txTo, nIn, flags):
13        return self.l.bitcoinconsensus_verify_script(scriptPubKey, c_uint(len(scriptPubKey)), 
14                txTo, c_uint(len(txTo)), 
15                c_uint(nIn), c_uint(flags))
16
17if __name__ == '__main__':
18    l = BitcoinConsensus('./src/.libs/libbitcoinconsensus.so')
19    infile = '/tmp/scripttest.txt'
20
21    print('API version', l.version())
22    assert(l.version() == 0)
23
24    with open(infile, 'r') as f:
25        for i,line in enumerate(f):
26            d = line.split(' ')
27            txTo = binascii.a2b_hex(d[1])
28            scriptPubKey = binascii.a2b_hex(d[2])
29            nIn = int(d[3])
30            nFlags = int(d[4]) # as CScript bitfield
31            rv = int(d[5])
32
33            # check
34            r = l.verify_script(scriptPubKey, txTo, nIn, nFlags)
35            if r != rv:
36                print('Mismatch at line %i, verify returns %i versus %i (nFlags %i)' % (i+1, r, rv, nFlags))
    • I pumped 1.2GB of verification operations through the above script, and no errors occurred.
    • I’ve also tried corrupting the scripts and/or transactions as well as nIn, resulting as expected in the function returning False but no crashes
      • Appending arbitrary data to txTo did not cause verification to fail: is this expected? Should it check whether the entire input is consumed?
    • I’ve stress-tested it by running in a loop for quite a while - no errors, memory usage was constant
  14. theuni commented at 8:24 pm on November 17, 2014: member

    @laanwj Thanks for testing!

    It sounds like a good idea to fail if the entire input isn’t consumed. That can be reflected in a new error (error codes still need to be hooked up here, I’ll do that once that PR is merged).

  15. theuni commented at 8:25 pm on November 17, 2014: member
    hah! I suppose I’ll do that now :)
  16. theuni force-pushed on Nov 17, 2014
  17. theuni commented at 11:59 pm on November 17, 2014: member

    Rebased and updated for the comments above. Fixed the missing copyright, wrong name for include guard, and unnecessary const arguments, and squashed those into the original commits.

    The last new commit adds error codes for busted txto’s, though it’s not very graceful. The errors aren’t related to the script, but to the tx.. so the codes are shoe-horned into the existing enum. Suggestions for a better approach are welcome.

  18. laanwj commented at 1:46 pm on November 18, 2014: member

    @theuni I’m not against a more-detailed failure reason in the API, but the current way is inconsistent. bitcoinconsensus.h should not depend on script_error.h which is an internal header.

    IMO if we’re going to do this we need a ScriptError just for external consumption, just like we did for the flags. This keeps bitcoinconsensus.h self-contained.

  19. sipa commented at 1:48 pm on November 18, 2014: member

    I think @theuni’s purpose with script_error was to be able to use it as both an internal and external header.

    However, I agree with @laanwj to keep the internal and external definitions completely separate - even if that means duplicating some code.

  20. laanwj commented at 1:54 pm on November 18, 2014: member

    I prefer have our API, at least at version 0, to be defined by one header.

    And yes - keeping the internal and external definitions completely separate is better, not least because they have diverged already: the new codes SCRIPT_ERR_TX_* errors are only used externally.

    BTW: If script_error.h is supposed to be an external header, it should not define the internal, non-API function const char* ScriptErrorString(const ScriptError error);

  21. jtimon commented at 2:06 pm on November 18, 2014: contributor
    Currently libconsensus only has script verification, but since we plan to add more things in the future, I don’t think it should be in script/, maybe have its own dir?
  22. sipa commented at 2:09 pm on November 18, 2014: member
    @jtimon agree
  23. theuni force-pushed on Nov 18, 2014
  24. theuni commented at 5:26 pm on November 18, 2014: member
    Updated after discussion on IRC. I went ahead and squashed it down since it was impossible to review commit-by-commit with things changing back and forth. Probably easier to review the new files wholly anyway. Sorry if that complicates at all.
  25. theuni commented at 5:33 pm on November 18, 2014: member
    @laanwj I missed your comment above about api version before pushing. Agreed. I’ll add that with the next changes pending review. @jtimon Agreed, but ok to change that as we go as more functionality is added?
  26. theuni commented at 0:51 am on November 19, 2014: member
    Briefly tested the .dll and .dylib enough to verify that they actually load and bitcoinconsensus_version() works, so presumably the objects themselves are in working order.
  27. laanwj commented at 8:21 am on November 19, 2014: member

    API looks good to me now. @theuni objdump -p output for the dll looks good to me; http://paste.ubuntu.com/9090362/ . Only dependence is on system DLLs

    • DLL Name: ADVAPI32.dll
    • DLL Name: GDI32.dll
    • DLL Name: KERNEL32.dll
    • DLL Name: msvcrt.dll
    • DLL Name: USER32.dll

    Only exported symbols are those two that we want:

    0Export Address Table -- Ordinal Base 1
1    [   0] +base[   1] 22c40 Export RVA
2    [   1] +base[   2] 21e10 Export RVA
3
4[Ordinal/Name Pointer] Table
5    [   0] bitcoinconsensus_verify_script
6    [   1] bitcoinconsensus_version

    Edit: test on windows completed succesfully. I executed the same above test in Python on windows - no problems. Tried to just load/unload the library a few times, just to try it out, no problems.

  28. laanwj commented at 8:54 am on November 19, 2014: member
    ACK commithash ae6bb2f7ff016df27456d544842ec6bc5f5c7cfa https://dev.visucore.com/bitcoin/acks/5235
  29. in src/script/bitcoinconsensus.h: in ae6bb2f7ff outdated 
    31+extern "C" {
32+#endif
33+
34+typedef enum bitcoinconsensus_txerror_t
35+{
36+    bitcoinconsensus_TX_ERR_OK = 0,
    laanwj commented at 10:06 am on November 19, 2014:
    Thinking about it IMO this OK should be generic: bitcoinconsensus_ERR_OK = 0. It doesn’t really matter whether this is a transaction OK or another error OK :-)
    theuni commented at 3:21 am on November 20, 2014:

    This was meant to imply that the tx was accepted by the api, whether or not the verification succeeded. bitcoinconsensus_ERR_OK is a bit hazy, as it could imply that bitcoinconsensus_verify_script() itself succeeded.

    Ultimately I think you’re right, we just need to make it clear in the docs that it means “verification was attempted”.

  30. in src/script/bitcoinconsensus.h: in ae6bb2f7ff outdated 
    29+
30+#ifdef __cplusplus
31+extern "C" {
32+#endif
33+
34+typedef enum bitcoinconsensus_txerror_t
    laanwj commented at 10:06 am on November 19, 2014:
    s/bitcoinconsensus_txerror_t/bitcoinconsensus_error_t/
  31. sipa commented at 10:41 am on November 19, 2014: member

    ACK.

    Compiling with -O2 -flto, and stripping results in an 83 kbyte .so file. Nice.

    It would be nice to have some proof of concept command-line tool that shows how to use the library.

  32. laanwj commented at 10:46 am on November 19, 2014: member
    @sipa Yes, now that we’re going to export libraries, we’re also going to need library documentation and examples. No need to do in this pull though. Created an issue #5311.
  33. jtimon commented at 11:02 am on November 19, 2014: contributor
    Well, I don’t see any reason not to do it now, specially when you had to squash anyway.
  34. jtimon commented at 11:37 am on November 19, 2014: contributor
    @laanwj “Appending arbitrary data to txTo did not cause verification to fail: is this expected? Should it check whether the entire input is consumed?” The txTo is only used for signature verification (ie if there weren’t different hashtypes we could just pass a hash and the script instead of txTo and nIn). So if the script contains script sigs and you change the txTo without updating the signature the script should become invalid.
  35. sipa commented at 11:39 am on November 19, 2014: member
    @jtimon That’s changed now; a check was added that the passed transaction is deserialized completely.
  36. sipa commented at 10:09 pm on November 19, 2014: member
    Needs rebase.
  37. build: make a distinction between static app ldflags and static lib ldflags 
    For windows builds, exe's are always static, but libs should still conform to
--enabled-shared and --enable-static.
    e0077de5de
  38. build: mingw needs libssp for hardening with dlls 811a765bef
  39. build: check visibility attributes f36a40f7fd
  40. build: remove internal/protected build attribute checks 
    They're not necessary, and not always supported. We only need to know about
hidden and default.
    ee64c53c1f
  41. theuni force-pushed on Nov 20, 2014
  42. theuni commented at 3:29 am on November 20, 2014: member
    rebased, squashed down some, and addressed @laanwj’s points. @jtimon I’d like to move around later to avoid any bikeshedding wrt naming. There are still a few things left to handle (docs/tests/samples), so let’s do it along with those.
  43. build: add libbitcoinconsensus files and hook up the lib build 
    Credit BlueMatt for libbitcoinsonsensus.h/cpp
    2cf5f16c25
  44. build: add --with-libs so that libs are optional cdd36c6c5c
  45. build: add quick consensus lib tests 
    They should be hooked up in other places as well, but this is a start.
    269efa30ed
  46. build: shared lib build should work reasonably well now 19df238a7b
  47. build: fix static dll link for mingw 
    dll's are no longer dynamically linked to libgcc/libstdc++/libssp
    9ed8979e29
  48. build: pad header for osx libs 
    This ensures that users of the lib will be able to mangle the paths to work
in their bundles.
    9eb5a5fbef
  49. laanwj merged this on Nov 20, 2014
  50. laanwj closed this on Nov 20, 2014
  51. laanwj referenced this in commit 9842ed465b on Nov 20, 2014
  52. theuni referenced this in commit 272ca0134b on Nov 20, 2014
  53. theuni referenced this in commit f618577029 on Nov 20, 2014
  54. laanwj referenced this in commit 216685a6df on Nov 21, 2014
  55. jameshilliard referenced this in commit 2d7216c187 on Apr 15, 2015
  56. reddink referenced this in commit 1cd1a5ba48 on May 27, 2020
  57. laanwj referenced this in commit 54b66a6e5f on Feb 12, 2021
  58. sidhujag referenced this in commit cd8f951240 on Feb 12, 2021
  59. DrahtBot locked this on Sep 8, 2021


theuni sipa laanwj Diapolo makhmet jtimon

Labels
Feature

Milestone
0.10.0

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-17 18:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me