0.18.1: Backports

[maflcko]

No description provided.

[laanwj]

utACK for 5935f01

But I guess it makes sense to keep this open for a while for other things that need to be backported to the 0.18 branch.

[fanquake]

Could include #14984.

[promag]

@MarcoFalke I won't open a PR to backport 14984 until you say so, it should be a clean cherry pick.

[promag]

Can you include #16122 and #16135?

[maflcko]

Done

[fanquake]

The GCC bug test backport needs to be fixed for the setup_common refactor.

test/compilerbug_tests.cpp:5:10: fatal error: test/setup_common.h: No such file or directory
 #include <test/setup_common.h>
          ^~~~~~~~~~~~~~~~~~~~~
compilation terminated.

[maflcko]

Ugh, thx. Fixed.

[practicalswift]

@MarcoFalke What makes #16205 important to backport?

[fanquake]

@practicalswift It's mentioned at the bottom of that PR: #16205 (comment).

Looks like this accidentally fixed multiple test failures with the cross-compiled windows binaries
I am going to backport this, since it turned out to be a bugfix

[maflcko]

Thanks for asking. Please let me know if there is a pull request missing for backport or something was backported in error.

Apart from a bunch one-line conflicts, the commits are all clean cherry-picks.

[fanquake]

Checked 5935f01 and ensured that -fstack-reuse=none was being added to CXXFLAGS when compiling with GCC:

  CC            = gcc
  CFLAGS        = -g -O2
  CPPFLAGS      =   -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2  -DHAVE_BUILD_INFO -D__STDC_FORMAT_MACROS
  CXX           = g++ -std=c++11
  CXXFLAGS      =   -fstack-reuse=none -Wstack-protector -fstack-protector-all  -Wall -Wextra -Wformat -Wvla -Wredundant-decls  -Wno-unused-parameter -Wno-implicit-fallthrough   -g -O2
  LDFLAGS       = -pthread  -Wl,-z,relro -Wl,-z,now -pie  
  ARFLAGS       = cr
...
(16035)root@69972490e9d9:/bitcoin# gcc --version
gcc (Debian 8.3.0-6) 8.3.0

Checked 3dbc7de, and that loaded wallets are shown as disabled.

Checked b55cbe8 and that the bitcoin.conf opening behaviour works as expected.

Checked 7ed1a60 and that it’s not possible to enter and execute commands in the console during startup.

Checked d80c558 and that you cannot cause a segfault using:

src/bitcoin-cli -regtest dumpwallet ~/downloads/wallet
src/bitcoin-cli -regtest importwallet ~/downloads/wallet

Checked d1f2611. Note that this is not a clean cherry-pick. #include <test/setup_common.h> has been fixed up to be #include <test/test_bitcoin.h> for 0.18.

Also checked that the test failed if you compile without -fstack-protector=none:

Running 1 test case...
Entering test module "Bitcoin Test Suite"
test/compilerbug_tests.cpp(8): Entering test suite "compilerbug_tests"
test/compilerbug_tests.cpp(30): Entering test case "gccbug_90348"
test/compilerbug_tests.cpp(38): error: in "compilerbug_tests/gccbug_90348": check check_zero(in, i) has failed
test/compilerbug_tests.cpp(38): error: in "compilerbug_tests/gccbug_90348": check check_zero(in, i) has failed
test/compilerbug_tests.cpp(38): error: in "compilerbug_tests/gccbug_90348": check check_zero(in, i) has failed
test/compilerbug_tests.cpp(38): error: in "compilerbug_tests/gccbug_90348": check check_zero(in, i) has failed
test/compilerbug_tests.cpp(30): Leaving test case "gccbug_90348"; testing time: 19263us
test/compilerbug_tests.cpp(8): Leaving test suite "compilerbug_tests"; testing time: 19286us
Leaving test module "Bitcoin Test Suite"; testing time: 19364us
*** 4 failures are detected in the test module "Bitcoin Test Suite"
make[3]: *** [Makefile:14472: test/compilerbug_tests.cpp.test] Error 1

Note that 592016b is not a clean cherry-pick. It has the removal of the assert_greater_than include, which is not in the referenced commit (f402012).

Note that 79745d1 does not contain the src/sync.cpp change from fa8f195.

I assume d9fc969 was backported because other test changes rely on it? Regardless, why not backport the other commit (b679785) from # 15826? I’d have thought backporting the documentation would also be useful.

Checked 2800b3d and that wallets still appear in the GUI.

Checked 715da91 and that the warning is removed on a Ubuntu Bionic system. Given I'm running in virtualbox, I don't think I can see the same improvements that others saw in #16254.

Other commits I looked over the backported code.

Currently seeing failures running the functional tests. Have commented inline.

[fanquake]

Traceback (most recent call last):
  File "/Users/michael/github/bitcoin/test/functional/test_framework/test_framework.py", line 175, in main
    self.run_test()
  File "/Users/michael/github/bitcoin/test/functional/p2p_segwit.py", line 278, in run_test
    self.test_superfluous_witness()
  File "/Users/michael/github/bitcoin/test/functional/p2p_segwit.py", line 2076, in test_superfluous_witness
    assert_raises_rpc_error(-22, "TX decode failed", self.nodes[0].decoderawtransaction, serialize_with_bogus_witness(tx).hex())
AttributeError: 'bytes' object has no attribute 'hex'

[maflcko]

@fanquake .hex requires python3.5

[fanquake]

@MarcoFalke isn’t the 0.18 branch minimum 3.4.9?

[maflcko]

Bumped it, since 3.4 is EOL

[maflcko]

assume d9fc969 was backported because other test changes rely on it? Regardless, why not backport the other commit (b679785) from # 15826? I’d have thought backporting the documentation would also be useful.

Indeed. Good point on the doc, added as backport.

[promag]

Commit 79745d1 is missing:

--- a/src/sync.cpp
+++ b/src/sync.cpp
@@ -47,7 +47,9 @@ struct CLockLocation {

     std::string ToString() const
     {
-        return mutexName + "  " + sourceFile + ":" + itostr(sourceLine) + (fTry ? " (TRY)" : "");
+        return strprintf(
+            "%s %s:%s%s (in thread %s)",
+            mutexName, sourceFile, itostr(sourceLine), (fTry ? " (TRY)" : ""), m_thread_name);
     }

 private:

Is there a reason?

Edit: if the goal is to just replace fprintf then it's all good.

[promag]

ACK bcb27d7, I've verified all commits but just built each unclean cherry pick (which are trivial).

But I guess it makes sense to keep this open for a while for other things that need to be backported to the 0.18 branch.

Edit: I think this PR is now substantial enough.

[fanquake]

ACK bcb27d7

Had a quick look over af25a75, and confirmed that the functional tests are now running (using 3.5.6) after bcb27d7. Full test output is here. Note that p2p_invalid_messages.py fails quite often on macOS, so isn't an issue here.

I agree that this is turning into a lot to review and test, so would suggest we merge this soon (assuming some more reviewers) without piling to much more in.

[luke-jr]

These should be a clean merge, not cherry-picked:

Bugfix: test/functional/rpc_psbt: Remove check for specific error mes…
Bugfix: test/functional/rpc_psbt: Correct test description comment …

[maflcko]

@luke-jr I used cherry-picks for consistency. (Most other backports are cherry-picks)

[luke-jr]

Merges should be used whenever possible. (Ideally, it would be nice if we could get to the point where most fixes are clean merges.)

[maflcko]

I agree, but sometimes it is not clear in advance what is a fix and needs to be backported. Sounds off-topic for this pull request and could be raised in an irc meeting?

[luke-jr]

The incorrect cherry-picks are part of this PR :)

[maflcko]

This has been the policy for years, afaic. So to change it, it would have to be discussed. Also, I am not sure how that will interfere with the release process scripts of @laanwj.

[luke-jr]

It was never a policy. For a few recent examples, #15552 and #15524 were merged into 0.18 cleanly.

[ajtowns]

ACK bcb27d7 ; compiled and ran the tests; checked the commits match the commits they claim to, and that those commits are in master. didn't check that the commits haven't been reverted in master.

[laanwj]

ACK bcb27d7

Please don't merge yet as @fanquake would like to do this as his first merge to a branch 😄

This has been the policy for years, afaic. So to change it, it would have to be discussed. Also, I am not sure how that will interfere with the release process scripts of @laanwj.

It can handle either merges (only requirement is to have the correct PR # in the title of the merge commit), or the 'Github-Pull: #?(\d+)' metadata.