RFC: libbitcoinconsensus

[theuni]

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.

[sipa]

./autogen.sh
./configure --disable-wallet --without-gui
...
checking whether to build libraries... ./configure: line 23562: ],: command not found
yes
...

[theuni]

Er, sorry for the busted c/p. Fixed.

[laanwj]

This needs some additions to .gitignore:

?? src/.libs/
?? src/core/libbitcoinconsensus_la-transaction.lo
?? src/crypto/libbitcoinconsensus_la-ripemd160.lo
?? src/crypto/libbitcoinconsensus_la-sha1.lo
?? src/crypto/libbitcoinconsensus_la-sha2.lo
?? src/libbitcoinconsensus.la
?? src/libbitcoinconsensus_la-eccryptoverify.lo
?? src/libbitcoinconsensus_la-ecwrapper.lo
?? src/libbitcoinconsensus_la-hash.lo
?? src/libbitcoinconsensus_la-pubkey.lo
?? src/libbitcoinconsensus_la-uint256.lo
?? src/libbitcoinconsensus_la-utilstrencodings.lo
?? src/script/libbitcoinconsensus_la-bitcoinconsensus.lo
?? src/script/libbitcoinconsensus_la-interpreter.lo
?? src/script/libbitcoinconsensus_la-script.lo

[laanwj]

What is the reason for having const here on basic types like unsigned int?

[laanwj]

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]

No reason, will change. ACK on not using bool as well.

[laanwj]

Works for me.

I patched main.cpp to write out script verification operations

diff --git a/src/main.cpp b/src/main.cpp
index 2bff781..80433d8 100644
--- a/src/main.cpp
+++ b/src/main.cpp
@@ -1359,8 +1359,16 @@ void UpdateCoins(const CTransaction& tx, CValidationState &state, CCoinsViewCach

 bool CScriptCheck::operator()() const {
     const CScript &scriptSig = ptxTo->vin[nIn].scriptSig;
-    if (!VerifyScript(scriptSig, scriptPubKey, nFlags, CachingSignatureChecker(*ptxTo, nIn, cacheStore)))
+    bool rv;
+    if (!(rv=VerifyScript(scriptSig, scriptPubKey, nFlags, CachingSignatureChecker(*ptxTo, nIn, cacheStore))))
         return error("CScriptCheck() : %s:%d VerifySignature failed", ptxTo->GetHash().ToString(), nIn);
+
+    CDataStream ssTx(SER_NETWORK, PROTOCOL_VERSION);
+    ssTx << *ptxTo;
+    std::string strTxTo = HexStr(ssTx.begin(), ssTx.end());
+    std::string strScriptPubKey = HexStr(scriptPubKey.begin(), scriptPubKey.end());
+    printf("verify_script %s %s %i %i %i\n", strTxTo.c_str(), strScriptPubKey.c_str(), nIn, nFlags, rv);
+
     return true;
 }

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

from __future__ import print_function, division
import json, binascii, os
import ctypes
from ctypes import c_uint

class BitcoinConsensus(object):
    def __init__(self, path):
        self.l = ctypes.CDLL(path)

    def version(self):
        return self.l.bitcoinconsensus_version()

    def verify_script(self, scriptPubKey, txTo, nIn, flags):
        return self.l.bitcoinconsensus_verify_script(scriptPubKey, c_uint(len(scriptPubKey)), 
                txTo, c_uint(len(txTo)), 
                c_uint(nIn), c_uint(flags))

if __name__ == '__main__':
    l = BitcoinConsensus('./src/.libs/libbitcoinconsensus.so')
    infile = '/tmp/scripttest.txt'

    print('API version', l.version())
    assert(l.version() == 0)

    with open(infile, 'r') as f:
        for i,line in enumerate(f):
            d = line.split(' ')
            txTo = binascii.a2b_hex(d[1])
            scriptPubKey = binascii.a2b_hex(d[2])
            nIn = int(d[3])
            nFlags = int(d[4]) # as CScript bitfield
            rv = int(d[5])

            # check
            r = l.verify_script(scriptPubKey, txTo, nIn, nFlags)
            if r != rv:
                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

[theuni]

@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).

[theuni]

hah! I suppose I'll do that now :)

[theuni]

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.

[laanwj]

@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.

[sipa]

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.

[laanwj]

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);

[jtimon]

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?

[sipa]

@jtimon agree

[theuni]

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.

[theuni]

@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?

[theuni]

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.

[laanwj]

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:

Export Address Table -- Ordinal Base 1
    [   0] +base[   1] 22c40 Export RVA
    [   1] +base[   2] 21e10 Export RVA

[Ordinal/Name Pointer] Table
    [   0] bitcoinconsensus_verify_script
    [   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.

[laanwj]

ACK commithash ae6bb2f7ff016df27456d544842ec6bc5f5c7cfa https://dev.visucore.com/bitcoin/acks/5235

[laanwj]

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]

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".

[sipa]

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.

[jtimon]

@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.

[sipa]

@jtimon That's changed now; a check was added that the passed transaction is deserialized completely.

[sipa]

Needs rebase.

[theuni]

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.