Omnibus

[roconnor-blockstream]

Remove boost and /nix/store stuff from the coverage report.

I think there is a better way of fixing the boost stuf using
geninfo_adjust_src_path, but I couldn't get it to work.

[apoelstra]

Thanks!

Tested ACK 50ab8c2

[apoelstra]

This is good to merge, unless you want to delete the commented-out line.

Not really relatedly, I am using this locally

diff --git a/elements.nix b/elements.nix
index d5c1a58..7951f09 100644
--- a/elements.nix
+++ b/elements.nix
@@ -1,5 +1,5 @@
 args@
-{ stdenv, fetchFromGitHub ? null, pkg-config, autoreconfHook, db48, boost, zeromq
+{ stdenv, fetchFromGitHub ? null, fetchpatch, pkg-config, autoreconfHook, db48, boost, zeromq
 , zlib, miniupnpc, qtbase ? null, qttools ? null, util-linux ? null, hexdump ? null, protobuf, python3, qrencode, libevent
 , lcov ? null
 , lib, writeText
@@ -33,7 +33,11 @@ stdenv.mkDerivation rec {
           sha256 = "sha256-UNjYkEZBjGuhkwBxSkNXjBBcLQqoan/afCLhoR2lOY4=";
         };

-  patches = [ ./elements-lcov.patch ];
+  patches = [
+      ./elements-lcov.patch
+      (fetchpatch "https://github.com/bitcoin/bitcoin/commit/398768769f85cc1b6ff212ed931646b59fa1acd6.patch")
+      (fetchpatch "https://github.com/bitcoin/bitcoin/commit/af862661654966d5de614755ab9bd1b5913e0959.patch")
+  ];

   postPatch = optionals (doCheck)
   ''

which pulls in the two patches from Bitcoin that are needed for Elements master to build. Doing this in elements.nix is much easier than keeping track of these patches and cherry-picking them onto every single branch.

I guess, to do this properly we should hide these behind some boolean flag, since they might not apply to all branches.

[roconnor-blockstream]

I've added your fetchpatch stuff to this PR.

[apoelstra]

@roconnor-blockstream can you add a boolean flag to allow disabling these patches? When I run this with a branch that already has them, it detects that the patch is already applied but then fails. (And my local branches still need the patches, annoyingly, so that I can develop interactively in a nix-shell using the actual git repo as source. If I run patchPhase then it dirties up my git worktree.)

[roconnor-blockstream]

Done.

[apoelstra]

Also, on line https://github.com/roconnor-blockstream/elements-nix/blob/main/elements.nix#L57 we set

--with-sanitizers=address,fuzzer,undefined

For fast fuzzing (i.e. trying to get high coverage without trying to fail quickly) we want this to be

--with-sanitizers=fuzzer

and then for running the seeds we really want

--with-sanitizers=address,fuzzer,undefined,integer

which matches what's in Elements CI.

[apoelstra]

Maybe withFuzz should be a string argument which must be one of none, fast, full. Where none disables the fuzzer completely, fast turns it on with just fuzzer, and full turns on all the sanitizers. (I'm not sure that we need an option corresponding to the current set of sanitizers.)

[apoelstra]

With full we might also need to set UBSAN_OPTIONS and ASAN_OPTIONS the way that they are set by test/fuzz/test_runner.py. Unsure.

[apoelstra]

Also would like a option to add the --enable-debug configure flag

[apoelstra]

In postPatch we should use sed -i to replace */include/ with include/ in tests/sanitizer_suppressions/ubsan`.

It seems like something similar should be able to fix the issue with lcov thinking that include files are in src/include rather than in the system, but I couldn't find other instances of /include/ in the build system. It may be internal to lcov. But the underlying issue seems to be that clang itself reports include paths as starting with include/ when it's run from within the stdenv wrapper, which uses the -isystem flag to add include paths. After experimenting a bit it seems like both clang and gcc treat -I/-isystem paths differently from the builtin FHS /usr/include-type paths.

[roconnor-blockstream]

A while back I spend quite a bit of time fiddling with clang's -I / -isystem stuff. Unfortunately I didn't take notes, but in the end I couldn't get it to work. I was convinced there was some sort of internal clang bug.

[roconnor-blockstream]

I have

feature_fedpeg.py --legacy-wallet                    | ✖ Failed  | 33 s
feature_fedpeg.py --post_transition --legacy-wallet  | ✖ Failed  | 30 s
feature_fedpeg.py --pre_transition --legacy-wallet   | ✖ Failed  | 33 s
feature_trim_headers.py                              | ✖ Failed  | 1328 s
wallet_keypool_topup.py --legacy-wallet              | ✖ Failed  | 134 s

When running --arg doFunctionalTests true. I this expected?

[apoelstra]

I think so. I see this too, before and after this change, on Elements master.

[roconnor-blockstream]

Should we drop the GCC13Patches from this PR?

[apoelstra]

No, they're still needed for Elements 0.21. But we should turn them off by default I think.

[apoelstra]

...we also need boost 1.75 for 0.21. It may just not be worth trying to support 0.21 anymore. I use in it my local CI but I can maintain local patches for that.

[roconnor-blockstream]

I'm willing to accept a PR for boost 1.75, but I'm not willing to construct it myself.

[roconnor-blockstream]

I think we are good to merge if you are happy with 8f55670.

[apoelstra]

Code looks great -- have not tried the fuzz stuff yet. Let me try the basic functionality with 0.21, 22 and 23 and then I'll ack.

[apoelstra]

ACK 8f55670

Was able to build 3 different major versions of Elements with it. Played with the sanitizers a bit -- turned on asan but it was too slow to work with the rust-elements tests. But it did build and had an observable effect, which is pretty cool.