Add generic tree-walking algorithm, and use for ToScript/ToString #83
Conversation
77950a3 to
e2e18ee
Compare
darosior
left a comment
There was a problem hiding this comment.
A compilation failures i didn't catch before trying to test it with Bitcoin Core.
4e47f27 to
de22eb1
Compare
darosior
left a comment
There was a problem hiding this comment.
Couple more comments after checking with my unit tests.
darosior
left a comment
There was a problem hiding this comment.
Another compilation failure that for some reason i didn't encounter before.
bitcoin/script/miniscript.h
Outdated
| // Invoke upfn with the last node.subs.size() elements of results as input. | ||
| assert(results.size() >= node.subs.size()); | ||
| std::optional<Result> result{upfn(std::move(stack.back().state), node, | ||
| Span{results}.last(node.subs.size()))}; |
There was a problem hiding this comment.
| Span{results}.last(node.subs.size()))}; | |
| Span<Result>{results}.last(node.subs.size()))}; |
There was a problem hiding this comment.
That really shouldn't be necessary. Do you have an old span.h by any chance, or not using -std=c++17?
There was a problem hiding this comment.
Interestingly this only happened when building with fuzz enabled. I'll retry both builds from scratch and report.
There was a problem hiding this comment.
So i still have this error (only) when building the fuzz target.
$ make V=1
Making all in src
make[1]: Entering directory '/home/darosior/projects/bitcoin/src'
make[2]: Entering directory '/home/darosior/projects/bitcoin/src'
make[3]: Entering directory '/home/darosior/projects/bitcoin'
make[3]: Leaving directory '/home/darosior/projects/bitcoin'
clang++ -std=c++17 -DHAVE_CONFIG_H -I. -I../src/config -DABORT_ON_FAILED_ASSUME -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -I. -I./minisketch/include -I./secp256k1/include -I./univalue/include -I/home/darosior/projects/bitcoin/../db4/include -I/usr/include -I./leveldb/include -I./leveldb/helpers/memenv -DHAVE_BUILD_INFO -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection -Wall -Wextra -Wgnu -Wformat -Wformat-security -Wvla -Wshadow-field -Wthread-safety -Wrange-loop-analysis -Wredundant-decls -Wunused-member-function -Wdate-time -Wconditional-uninitialized -Woverloaded-virtual -Wsuggest-override -Wunreachable-code-loop-increment -Wimplicit-fallthrough -Wno-unused-parameter -Wno-self-assign -Wno-deprecated-copy -fsanitize=fuzzer -fPIE -g -O2 -MT test/fuzz/fuzz-miniscript_decode.o -MD -MP -MF test/fuzz/.deps/fuzz-miniscript_decode.Tpo -c -o test/fuzz/fuzz-miniscript_decode.o `test -f 'test/fuzz/miniscript_decode.cpp' || echo './'`test/fuzz/miniscript_decode.cpp
In file included from test/fuzz/miniscript_decode.cpp:8:
./script/miniscript.h:425:30: error: member reference base type 'Span' is not a structure or union
Span{results}.last(node.subs.size()))};
~~~~~~~~~~~~~^~~~~
1 error generated.
make[2]: *** [Makefile:15058: test/fuzz/fuzz-miniscript_decode.o] Error 1
make[2]: Leaving directory '/home/darosior/projects/bitcoin/src'
make[1]: *** [Makefile:17529: all-recursive] Error 1
make[1]: Leaving directory '/home/darosior/projects/bitcoin/src'
make: *** [Makefile:823: all-recursive] Error 1
$ clang++ --version
Debian clang version 11.0.1-2
There was a problem hiding this comment.
@darosior Do you have a branch with everything integrated (bitcoin core, miniscript with this PR and fuzz test) that I can play around with?
There was a problem hiding this comment.
Here you go: https://github.com/darosior/bitcoin/tree/miniscript_pr_83. Based on the watchonly PR (darosior/bitcoin#5).
I just re-tried on this branch tip and i can build without issue with:
./autogen.sh
./configure --enable-debug --enable-c++17 --without-gui --disable-bench "$@" BDB_LIBS="-L${BDB_PREFIX}/lib -ldb_cxx-4.8" BDB_CFLAGS="-I${BDB_PREFIX}/include"
make -j20But not with
./autogen.sh
./configure --enable-fuzz --enable-c++17 --with-sanitizers=fuzzer BDB_LIBS="-L${BDB_PREFIX}/lib -ldb_cxx-4.8" BDB_CFLAGS="-I${BDB_PREFIX}/include" CC=clang CXX=clang++
make -j20And the g++ used to build without the fuzz target:
g++ (Debian 10.3.0-11) 10.3.0
There was a problem hiding this comment.
Very strange. I can build the fuzz tests without problem in your branch (clang++ --version => 12.0.0-3ubuntu1~21.04.2). I'll add the Span<Result> if that helps, but I'd still like to understand what causes this.
There was a problem hiding this comment.
Trying with another clang version now.
EDIT: same error with clang++-9.
EDIT 2: so this is fixed starting with clang++-12. To be precise i used clang+llvm-12.0.1-x86_64-linux-gnu-ubuntu-16.04.tar.xz from https://github.com/llvm/llvm-project/releases/tag/llvmorg-12.0.1.
de22eb1 to
6c9e436
Compare
6c9e436 to
b2605c6
Compare
There was a problem hiding this comment.
ACK b2605c6 -- in addition to the previous code review and testing, for the past few days i've been fuzzing this both with the target from #83 and Bitcoin Core's descriptor_parse with darosior/bitcoin#5 applied. I have not encountered any failure to roundtrip nor any crash in ToString()/ToString().
sanket1729
left a comment
There was a problem hiding this comment.
Post merge code review ACK
| case NodeType::WRAP_S: return (CScript() << OP_SWAP) + std::move(subs[0]); | ||
| case NodeType::WRAP_C: return std::move(subs[0]) + CScript() << (verify ? OP_CHECKSIGVERIFY : OP_CHECKSIG); | ||
| case NodeType::WRAP_D: return (CScript() << OP_DUP << OP_IF) + std::move(subs[0]) + (CScript() << OP_ENDIF); | ||
| case NodeType::WRAP_V: return std::move(subs[0]) + (node.subs[0]->GetType() << "x"_mst ? (CScript() << OP_VERIFY) : CScript()); |
There was a problem hiding this comment.
Another way to do this. Directly change the opcode to verify version in case the type is not "x"
- OP_EQUAL -> OP_EQUALVERIFY
- OP_CHECKSIG -> OP_CHECKSIGVERIFY
- OP_CHECKMULTISIG -> OP_CHECKMULTISIGVERIFY
This avoids keeping track of the state variable verify. Just a note, no need to modify the existing implementation.
There was a problem hiding this comment.
You'd still need verify for WRAP_S and AND_V though?
No description provided.