0.18.1: Backports

MarcoFalke commented at 2:10 pm on May 16, 2019: member

build with -fstack-reuse=none

Github-Pull: #15983
Rebased-From: faf38bc056e523485520f98f3f725c583a3b89bf

5935f0126e

MarcoFalke added the label Backport on May 16, 2019

MarcoFalke added this to the milestone 0.18.1 on May 16, 2019

laanwj commented at 7:47 pm on May 16, 2019: member

utACK for 5935f0126ef37175a8fbfee017b470af13aad46d

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.

net: Rename ::fRelayTxes to ::g_relay_txes

This helps to distinguish it from CNode::fRelayTxes and avoid bugs like
425278d17bd0edf8a3a7cc81e55016f7fd8e7726

Github-Pull: #15990
Rebased-From: fa1dce7329d3e74d46ab98b93772b1832a3f1819

9c1a607a09

test: Format predicate source as multiline on error

Github-Pull: #15990
Rebased-From: fa3872e7b4540857261aed948b94b6b2bfdbc3d1

8f215c7a27

test: Add test for p2p_blocksonly

Github-Pull: #15990
Rebased-From: fa320de79faaca2b088fcbe7f76701faa9bff236

3460555f47

doc: Mention blocksonly in reduce-traffic.md, unhide option

Github-Pull: #15990
Rebased-From: fa8ced32a60dea37ac169241cf9a1f708ef46c4b

890a92eba8

fanquake commented at 3:15 am on May 17, 2019: member

Could include #14984.

Doc: remove text about txes always relayed from -whitelist

Updates text since -whitelistforcerelay was set to false by default in
PR #15193.

Github-Pull: #15890
Rebased-From: e0bb2799992afe88e6f4efc6d90ed82ddf1ec5ec

eb85ee62b3

Install bitcoin-wallet manpage.

This change marks the already-existing bitcoin-wallet.1 manpage file for
installation together with the others.  Previously, only bitcoind.1,
bitcoin-cli.1, bitcoin-tx.1 and bitcoin-qt.1 would be installed.

Github-Pull: #15947
Rebased-From: 00d110463aed12ecdc6e9c2bf47d9ef61d19fa9d

a635377b62

promag commented at 10:10 am on May 18, 2019: member

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

Show loaded wallets as disabled in open menu instead of nothing

Github-Pull: #15957
Rebased-From: c3ef63a52f304a600fff1f9c7caa5cb804d41d43

3dbc7def0f

Disallow extended encoding for non-witness transactions

Github-Pull: #14039
Rebased-From: bb530efa1872ec963417f61da9a95185c7a7a7d6

206f5ee875

Fix missing input template by making minimal tx

Github-Pull: #15893
Rebased-From: 25b078658139c1aea58393a32ac5a79144d8d140

5a58ddb6d5

Add test for superfluous witness record in deserialization

Github-Pull: #15893
Rebased-From: cc556e4a30b4a32eab6722f590489d89b2875de3

86031083c7

Disallow extended encoding for non-witness transactions (take 3)

Github-Pull: #16021
Rebased-From: fa2b52af32f6a4b9c22c270f36e92960c29ef364

b6c1f9478f

promag commented at 4:06 pm on June 6, 2019: member

Can you include #16122 and #16135?

MarcoFalke commented at 8:02 am on June 7, 2019: member

Done

fanquake commented at 8:26 am on June 7, 2019: member

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

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

qt: fix opening bitcoin.conf via Preferences on macOS; see #15409

Github-Pull: #16044
Rebased-From: 6e6494b3fb345848025494cb7a79c5bf8f35e417

b55cbe82d9

gui: Enable console line edit on setClientModel

Github-Pull: #16122
Rebased-From: 2d8ad2f99710a8981e33fe2d6ce834b0076c4e80

7ed1a60193

gui: Set progressDialog to nullptr

Github-Pull: #16135
Rebased-From: d2ae6be80f6a0156021bf8c9b9d17cd4966ddffc

d80c558e02

Add test for GCC bug 90348

Github-Pull: #15985
Rebased-From: 58e291cfad12fa85af87d093acfa7b44702e3521

d1f261150b

MarcoFalke force-pushed on Jun 7, 2019

MarcoFalke commented at 10:40 am on June 7, 2019: member

Ugh, thx. Fixed.

gui: Enable open wallet menu on setWalletController

Github-Pull: #16118
Rebased-From: 75485ef0962a53946f17b761c4445627b07e6eff

b2398240ff

MarcoFalke force-pushed on Jun 13, 2019

Fix RPC/pruneblockchain returned prune height

Github-Pull: #15991
Rebased-From: 97f517dd851450b1ede1eb6b20f77691883a7737

c80a498ae5

fixup: Fix prunning test

Github-Pull: #15991
Rebased-From: f402012ccfc596d7d94851dabbf386c278ff5335

592016ba18

MarcoFalke force-pushed on Jun 14, 2019

Add example 2nd arg to signrawtransactionwithkey

The RPC examples for signrawtransactionwithkey are missing the 2nd parameter.

Github-Pull: #16210
Rebased-From: 71fd628adafdeb2a4b343e0d51d7168cdb186312

d24d0ec056

fanquake commented at 1:04 pm on June 18, 2019: member

Can add #15899.

rpc: Switch touched RPCs to IsValidNumArgs

Github-Pull: #15899
Rebased-From: fa5c5cd141f0265a5693234690ac757b811157d8

bb36ac82ef

Bugfix: test/functional/rpc_psbt: Remove check for specific error message that depends on uncertain assumptions

When converttopsbt is called with a signed transaction, it either fails with "TX decode failed" if one or more inputs were segwit, or "Inputs must not have scriptSigs and scriptWitnesses" otherwise.
Since no effort is made by the test to ensure the inputs are segwit or not, avoid checking the exact message used.
The error code is still checked to ensure it is of the correct kind of failure.

Github-Pull: #14818
Rebased-From: 097c4aa379f255639ce0084702693fa72a595d6b

966d8d0842

Bugfix: test/functional/rpc_psbt: Correct test description comment

Github-Pull: #14818
Rebased-From: c87fc71f7e9316bcc0653cd86c50177424b5b1f9

832eb4ff54

rpc: bugfix: Properly use iswitness in converttopsbt

Also explain the param in all RPCs

Github-Pull: #15899
Rebased-From: fa499b5f027f77c0bf13699852c8c06f78e27bef

0023c97890

tinyformat: Add doc to Bitcoin Core specific strprintf

Github-Pull: #16205
Rebased-From: fa72a64b90dc07a80b1ca6127eb50d8244dedc3b

f88959ba7c

Exceptions should be caught by reference, not by value.

Github-Pull: #16095
Rebased-From: ae7faf20d5fb3e2415ccadc37100dfc44aa0cd94

e29aa6e72e

scripted-diff: Replace fprintf with tfm::format

-BEGIN VERIFY SCRIPT-
sed -i --regexp-extended -e 's/fprintf\(std(err|out), /tfm::format(std::c\1, /g' $(git grep -l 'fprintf(' -- ':(exclude)src/crypto' ':(exclude)src/leveldb' ':(exclude)src/univalue' ':(exclude)src/secp256k1')
-END VERIFY SCRIPT-

fixup! scripted-diff: Replace fprintf with tfm::format

Github-Pull: #16205
Rebased-From: fac03ec43a15ad547161e37e53ea82482cc508f9

beb09f09b3

Replace remaining fprintf with tfm::format manually

Github-Pull: #16205
Rebased-From: fa8f195195945ce6258199af0461e3fbfbc1236d

79745d1752

test: Fixup creatmultisig documentation and whitespace

Github-Pull: #15831
Rebased-From: fad81d870aa6dd25d4fab5faad4c956ba364734a

13b3bb5644

test: Add test that addmultisigaddress fails for watchonly addresses

Github-Pull: #15831
Rebased-From: fab6a0a659bb856e4598af3e0679fc37d5239478

23ba460c1a

MarcoFalke force-pushed on Jun 21, 2019

Pure python EC

This removes the dependency on OpenSSL for the interaction tests, by providing a pure-Python
toy implementation of secp256k1.

Github-Pull: #15826
Rebased-From: 8c7b9324ca3f3ffb178bea56a96ea23f7e0383cb

d9fc969e71

Make and get the multisig redeemscript and destination in one function instead of two

Instead of creating a redeemScript with CreateMultisigRedeemscript and
then getting the destination with AddAndGetDestinationForScript, do
both in the same function.

CreateMultisigRedeemscript is changed to AddAndGetMultisigDestination.
It creates the redeemScript and returns it via an output parameter. Then
it calls AddAndGetDestinationForScript to add the destination to the
keystore and get the proper destination.

This allows us to inspect the public keys in the redeemScript before creating
the destination so that the correct destination is used when uncompressed
pubkeys are in the multisig.

Github-Pull: #16026
Rebased-From: a49503402b6bc21e3878e151c07529941d36aed0

e78007fc1a

MarcoFalke commented at 1:51 pm on June 21, 2019: member

This is ready for review/merge

fanquake commented at 10:59 am on June 23, 2019: member

Can you add #16231 as well? I will review after that.

gui: Fix open wallet menu initialization order

The menu must be created before connecting to aboutToShow signal.

Github-Pull: #16231
Rebased-From: 5224be5a3354e1a22ea4d7f0e40aadfccdf67064

2800b3d5c1

practicalswift commented at 4:39 pm on June 23, 2019: contributor

@MarcoFalke What makes #16205 important to backport?

fanquake commented at 2:58 am on June 24, 2019: member

@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

MarcoFalke commented at 12:56 pm on June 24, 2019: member

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.

Set AA_EnableHighDpiScaling attribute early

Qt docs: This attribute must be set before QGuiApplication is
constructed.

Github-Pull: #16254
Rebased-From: 099e4b9ad3d9967051d5c3d45c6315d1b30fea05

715da91e91

in test/functional/p2p_segwit.py:2076 in 715da91e91 outdated

2071+        self.nodes[0].generate(1)
2072+        unspent = next(u for u in self.nodes[0].listunspent() if u['spendable'] and u['address'].startswith('bcrt'))
2073+
2074+        raw = self.nodes[0].createrawtransaction([{"txid": unspent['txid'], "vout": unspent['vout']}], {self.nodes[0].getnewaddress(): 1})
2075+        tx = FromHex(CTransaction(), raw)
2076+        assert_raises_rpc_error(-22, "TX decode failed", self.nodes[0].decoderawtransaction, serialize_with_bogus_witness(tx).hex())

fanquake commented at 10:34 am on June 26, 2019:

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

MarcoFalke commented at 1:23 pm on June 26, 2019:

@fanquake .hex requires python3.5

fanquake commented at 1:24 pm on June 26, 2019:

@MarcoFalke isn’t the 0.18 branch minimum 3.4.9?

MarcoFalke commented at 1:33 pm on June 26, 2019:

Bumped it, since 3.4 is EOL

fanquake commented at 10:35 am on June 26, 2019: member

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

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

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

loaded_wallets

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

bitcoin_conf

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

Checked d80c558e026a4e500d5894d0c386378ac7a4d20f and that you cannot cause a segfault using:

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

Checked d1f261150b49a957138ef4920203f91f5a8c01b6. 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:

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

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

Note that 79745d1752d0d1a95f073a04d6f4b9715745cf17 does not contain the src/sync.cpp change from fa8f195195945ce6258199af0461e3fbfbc1236d.

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

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

listed_wallets

Checked 715da91e917af106d8d77fb6240f2adf84bca292 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.

ubuntu

Other commits I looked over the backported code.

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

Add comments to Python ECDSA implementation

Github-Pull: #15826
Rebased-From: b67978529ad02fc2665f2362418dc53db2e25e17

af25a757e0

MarcoFalke commented at 1:25 pm on June 26, 2019: member

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.

.python-version: Bump to 3.5.6

See also: dddd1d05d3df06865f5e0b1442d7425c0955de4e

bcb27d7b03

promag commented at 1:44 pm on June 26, 2019: member

Commit 79745d1752d0d1a95f073a04d6f4b9715745cf17 is missing:

 0--- a/src/sync.cpp
 1+++ b/src/sync.cpp
 2@@ -47,7 +47,9 @@ struct CLockLocation {
 3
 4     std::string ToString() const
 5     {
 6-        return mutexName + "  " + sourceFile + ":" + itostr(sourceLine) + (fTry ? " (TRY)" : "");
 7+        return strprintf(
 8+            "%s %s:%s%s (in thread %s)",
 9+            mutexName, sourceFile, itostr(sourceLine), (fTry ? " (TRY)" : ""), m_thread_name);
10     }
11
12 private:

Is there a reason?

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

promag commented at 1:55 pm on June 26, 2019: member

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 approved

fanquake commented at 8:04 am on June 27, 2019: member

ACK bcb27d7b03d1e7a4404c04859f8146d8bc7c360a

Had a quick look over af25a757e077f907a8ce1cb676bd4908ed638e14, and confirmed that the functional tests are now running (using 3.5.6) after bcb27d7b03d1e7a4404c04859f8146d8bc7c360a. 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 changes_requested

luke-jr commented at 3:21 pm on July 1, 2019: member

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

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

MarcoFalke assigned laanwj on Jul 1, 2019

MarcoFalke commented at 3:25 pm on July 1, 2019: member

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

luke-jr commented at 3:30 pm on July 1, 2019: member

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

MarcoFalke commented at 3:33 pm on July 1, 2019: member

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 commented at 4:04 pm on July 1, 2019: member

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

MarcoFalke commented at 4:08 pm on July 1, 2019: member

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 commented at 4:38 pm on July 1, 2019: member

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

SkippydooDougstrong7772 approved

fanquake deleted a comment on Jul 1, 2019

ajtowns commented at 2:04 pm on July 3, 2019: member

ACK bcb27d7b03d1e7a4404c04859f8146d8bc7c360a ; 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 commented at 2:23 pm on July 3, 2019: member

ACK bcb27d7b03d1e7a4404c04859f8146d8bc7c360a

Please don’t merge yet as @fanquake would like to do this as his first merge to a branch :smile:

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.

MarcoFalke assigned fanquake on Jul 3, 2019

MarcoFalke unassigned laanwj on Jul 3, 2019

fanquake merged this on Jul 4, 2019

fanquake closed this on Jul 4, 2019

fanquake referenced this in commit 629c7b029c on Jul 4, 2019

DrahtBot locked this on Dec 16, 2021

0.18.1: Backports #16035