From 6a238884112e2845212e2f26e2426edf12a30d0d Mon Sep 17 00:00:00 2001 From: Samuel Dobson Date: Wed, 8 Jan 2020 11:29:48 +1300 Subject: [PATCH 01/19] Merge #17677: Activate watchonly wallet behavior for LegacySPKM only e1e1442f3eadc1d139380e71c1b60b86d8d6bdee Activate no-privkey -> ISMINE_WATCH_ONLY behavior for LegacySPKM only (Gregory Sanders) Pull request description: Slight cleanup following https://github.com/bitcoin/bitcoin/pull/16944 This should allow future scriptpubkeymans to transparently work, since the current plan is to have ismine always be spendable. ACKs for top commit: achow101: ACK e1e1442f3eadc1d139380e71c1b60b86d8d6bdee Sjors: Code review ACK e1e1442f3eadc1d139380e71c1b60b86d8d6bdee meshcollider: Code review ACK e1e1442f3eadc1d139380e71c1b60b86d8d6bdee Tree-SHA512: c0a86587d33b8b1646494a5cb0bf8681ee4a88e6913918157746943a0996b501903e0e6ee954cf04154c1e0faee0cbb375c74ca789f46ba9244eb5296632b042 --- src/wallet/wallet.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 42ff8da965..bdef69003b 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2638,8 +2638,8 @@ std::map> CWallet::ListCoins() const std::vector lockedCoins; ListLockedCoins(lockedCoins); - // Include watch-only for wallets without private keys - const bool include_watch_only = IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS); + // Include watch-only for LegacyScriptPubKeyMan wallets without private keys + const bool include_watch_only = GetLegacyScriptPubKeyMan() && IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS); const isminetype is_mine_filter = include_watch_only ? ISMINE_WATCH_ONLY : ISMINE_SPENDABLE; for (const COutPoint& output : lockedCoins) { auto it = mapWallet.find(output.hash); From 9d0a82b02e7df269b911e00669cd05a9c697aea1 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Sun, 12 Apr 2020 19:33:27 -0400 Subject: [PATCH 02/19] Merge #18593: test: complete impl. of msg_merkleblock and wait_for_merkleblock 854382885f18aa9a95cdde3d11591b05c305ad3f refactor: test: improve wait_for{header,merkleblock} interface (Sebastian Falbesoner) 1356a45ef042e7bd3d539fbb606d6b1be547d00f test: complete impl. of msg_merkleblock and wait_for_merkleblock (Sebastian Falbesoner) Pull request description: Implements the missing initialization/serialization methods for `msg_merkleblock`, based on the already present class `CMerkleBlock`. Also changes the method `wait_for_merkleblock()` to be more precise by waiting for a merkleblock with a specified blockhash instead of an arbitrary one. In the BIP37 test `p2p_filter.py`, this new method is used to make the test of receiving merkleblock and tx if a filter is set to be more precise, by checking if they also arrive in the right order. In the course of this PR, also the interface for the methods `wait_for_merkleblock()` and `wait_for_header()` are improved to take a hex string instead of an integer, which is more typesafe and less of a burden to the caller. ACKs for top commit: MarcoFalke: ACK 854382885f18aa9a95cdde3d11591b05c305ad3f Tree-SHA512: adaf0ac728ef0b9929cb417a7a7b4c1346c400b2d365bf6914515c67b6cfe8f4a7ecc62fb514afdce9792f0bed833416f6bca6b9620f3d5dcdc66e4d5b0b7ea3 --- test/functional/p2p_filter.py | 14 ++++---------- test/functional/p2p_invalid_locator.py | 2 +- test/functional/test_framework/messages.py | 15 ++++++++++++++- test/functional/test_framework/mininode.py | 7 +++---- 4 files changed, 22 insertions(+), 16 deletions(-) diff --git a/test/functional/p2p_filter.py b/test/functional/p2p_filter.py index a22ee91483..99e6ed7c1d 100755 --- a/test/functional/p2p_filter.py +++ b/test/functional/p2p_filter.py @@ -12,10 +12,7 @@ from test_framework.messages import ( msg_getdata, msg_filterload, ) -from test_framework.mininode import ( - P2PInterface, - mininode_lock, -) +from test_framework.mininode import P2PInterface from test_framework.test_framework import BitcoinTestFramework @@ -68,18 +65,15 @@ class FilterTest(BitcoinTestFramework): filter_address = self.nodes[0].decodescript(filter_node.watch_script_pubkey)['addresses'][0] self.log.info('Check that we receive merkleblock and tx if the filter matches a tx in a block') - filter_node.merkleblock_received = False block_hash = self.nodes[0].generatetoaddress(1, filter_address)[0] txid = self.nodes[0].getblock(block_hash)['tx'][0] + filter_node.wait_for_merkleblock(block_hash) filter_node.wait_for_tx(txid) - assert filter_node.merkleblock_received self.log.info('Check that we only receive a merkleblock if the filter does not match a tx in a block') - with mininode_lock: - filter_node.last_message.pop("merkleblock", None) filter_node.tx_received = False - self.nodes[0].generatetoaddress(1, self.nodes[0].getnewaddress()) - filter_node.wait_for_merkleblock() + block_hash = self.nodes[0].generatetoaddress(1, self.nodes[0].getnewaddress())[0] + filter_node.wait_for_merkleblock(block_hash) assert not filter_node.tx_received self.log.info('Check that we not receive a tx if the filter does not match a mempool tx') diff --git a/test/functional/p2p_invalid_locator.py b/test/functional/p2p_invalid_locator.py index c8c752d1f7..15770819c9 100755 --- a/test/functional/p2p_invalid_locator.py +++ b/test/functional/p2p_invalid_locator.py @@ -34,7 +34,7 @@ class InvalidLocatorTest(BitcoinTestFramework): msg.locator.vHave = [int(node.getblockhash(i - 1), 16) for i in range(block_count, block_count - (MAX_LOCATOR_SZ), -1)] node.p2p.send_message(msg) if type(msg) == msg_getheaders: - node.p2p.wait_for_header(int(node.getbestblockhash(), 16)) + node.p2p.wait_for_header(node.getbestblockhash()) else: node.p2p.wait_for_block(int(node.getbestblockhash(), 16)) diff --git a/test/functional/test_framework/messages.py b/test/functional/test_framework/messages.py index 47bca27863..a79a3954ba 100755 --- a/test/functional/test_framework/messages.py +++ b/test/functional/test_framework/messages.py @@ -1819,10 +1819,23 @@ class msg_headers2: return "msg_headers2(headers=%s)" % repr(self.headers) class msg_merkleblock: + __slots__ = ("merkleblock",) command = b"merkleblock" + def __init__(self, merkleblock=None): + if merkleblock is None: + self.merkleblock = CMerkleBlock() + else: + self.merkleblock = merkleblock + def deserialize(self, f): - pass # Placeholder for now + self.merkleblock.deserialize(f) + + def serialize(self): + return self.merkleblock.serialize() + + def __repr__(self): + return "msg_merkleblock(merkleblock=%s)" % (repr(self.merkleblock)) class msg_filterload: diff --git a/test/functional/test_framework/mininode.py b/test/functional/test_framework/mininode.py index bbb7463d17..fa028d3f4d 100755 --- a/test/functional/test_framework/mininode.py +++ b/test/functional/test_framework/mininode.py @@ -473,18 +473,17 @@ class P2PInterface(P2PConnection): last_headers = self.last_message.get('headers') if not last_headers: return False - return last_headers.headers[0].rehash() == blockhash + return last_headers.headers[0].rehash() == int(blockhash, 16) self.wait_until(test_function, timeout=timeout) - def wait_for_merkleblock(self, timeout=60): + def wait_for_merkleblock(self, blockhash, timeout=60): def test_function(): assert self.is_connected last_filtered_block = self.last_message.get('merkleblock') if not last_filtered_block: return False - # TODO change this method to take a hash value and only return true if the correct block has been received - return True + return last_filtered_block.merkleblock.header.rehash() == int(blockhash, 16) wait_until(test_function, timeout=timeout, lock=mininode_lock) From 2699f446739bf3092c41099f9d1de692f0c21799 Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Wed, 15 Jul 2020 13:51:02 +0200 Subject: [PATCH 03/19] Merge #18882: build: fix -Wformat-security check when compiling with GCC 6cef3652d143a1dddad1254cab0953561d24c1fa build: fix -Wformat-security check when compiling with GCC (fanquake) Pull request description: GCC expects `-Wformat` to be passed with [`-Wformat-security`](https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html), which means when we test for it in configure it currently fails: ```bash checking whether C++ compiler accepts -Wformat-security... no ... configure:15907: checking whether C++ compiler accepts -Wformat-security configure:15926: g++ -std=c++11 -c -g -O2 -Werror -Wformat-security conftest.cpp >&5 cc1plus: error: '-Wformat-security' ignored without '-Wformat' [-Werror=format-security] cc1plus: all warnings being treated as errors ``` and never gets added to our CXX flags. Note that Clang does not have this requirement and the check is working correctly there. The change in this PR is the simple fix, however we might want to consider using something like `-Wformat=2` in future, which in GCC is equivalent to `-Wformat -Wformat-nonliteral -Wformat-security -Wformat-y2k.` and similar [in Clang](https://clang.llvm.org/docs/DiagnosticsReference.html#wformat-2). ACKs for top commit: practicalswift: ACK 6cef3652d143a1dddad1254cab0953561d24c1fa laanwj: ACK 6cef3652d143a1dddad1254cab0953561d24c1fa Tree-SHA512: f9230d42af39f85ea9d2f55dbbebd2bae4740fe59b0da2e092af3ac9ef7e6799d3a4cf83eb64574c63982e5f6b14e226d44c84fa0335255d65c9947d86a1ea38 --- configure.ac | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/configure.ac b/configure.ac index 3750127ea4..017e2e00cd 100644 --- a/configure.ac +++ b/configure.ac @@ -441,11 +441,11 @@ if test "x$CXXFLAGS_overridden" = "xno"; then AX_CHECK_COMPILE_FLAG([-Wall],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wall"],,[[$CXXFLAG_WERROR]]) AX_CHECK_COMPILE_FLAG([-Wextra],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wextra"],,[[$CXXFLAG_WERROR]]) AX_CHECK_COMPILE_FLAG([-Wgnu],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wgnu"],,[[$CXXFLAG_WERROR]]) - AX_CHECK_COMPILE_FLAG([-Wformat],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wformat"],,[[$CXXFLAG_WERROR]]) + dnl some compilers will ignore -Wformat-security without -Wformat, so just combine the two here. + AX_CHECK_COMPILE_FLAG([-Wformat -Wformat-security],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wformat -Wformat-security"],,[[$CXXFLAG_WERROR]]) AX_CHECK_COMPILE_FLAG([-Wvla],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wvla"],,[[$CXXFLAG_WERROR]]) AX_CHECK_COMPILE_FLAG([-Wshadow-field],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wshadow-field"],,[[$CXXFLAG_WERROR]]) AX_CHECK_COMPILE_FLAG([-Wswitch],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wswitch"],,[[$CXXFLAG_WERROR]]) - AX_CHECK_COMPILE_FLAG([-Wformat-security],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wformat-security"],,[[$CXXFLAG_WERROR]]) AX_CHECK_COMPILE_FLAG([-Wthread-safety],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wthread-safety"],,[[$CXXFLAG_WERROR]]) AX_CHECK_COMPILE_FLAG([-Wrange-loop-analysis],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wrange-loop-analysis"],,[[$CXXFLAG_WERROR]]) AX_CHECK_COMPILE_FLAG([-Wredundant-decls],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wredundant-decls"],,[[$CXXFLAG_WERROR]]) From ee3eae3c9f042c3148efabba814e8278bfff43b2 Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Sun, 6 Sep 2020 19:35:13 +0200 Subject: [PATCH 04/19] Merge #19897: Change FILE_CHAR_BLOCKLIST to FILE_CHARS_DISALLOWED MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 637d8bce741213295bd9b9d1982cae663c701ba1 Change FILE_CHAR_BLOCKLIST to FILE_CHARS_DISALLOWED (Benoit Verret) Pull request description: Blocklist is ambiguous. It could mean a list of blocks. Example: "blocknotify" in the same file refers to Bitcoin blocks. ACKs for top commit: MarcoFalke: ACK 637d8bce741213295bd9b9d1982cae663c701ba1 laanwj: ACK 637d8bce741213295bd9b9d1982cae663c701ba1 — this is a clear variable name improvement theStack: ACK 637d8bce741213295bd9b9d1982cae663c701ba1 jonatack: ACK 637d8bce741213295bd9b9d1982cae663c701ba1 eriknylund: ACK 637d8bce741213295bd9b9d1982cae663c701ba1 promag: ACK 637d8bce741213295bd9b9d1982cae663c701ba1. Tree-SHA512: 028e7102eeaf61105736c55c119a7f5c05411f2b6715a7939c41cb9e8f13afb757bbb6e7a302b3aae21722e69dab91f6eff8099e5884d248299905b4c7687c02 --- test/functional/feature_notifications.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/functional/feature_notifications.py b/test/functional/feature_notifications.py index 9bcecb2d34..6547dd5311 100755 --- a/test/functional/feature_notifications.py +++ b/test/functional/feature_notifications.py @@ -16,7 +16,7 @@ from test_framework.util import ( # Windows disallow control characters (0-31) and /\?%:|"<> FILE_CHAR_START = 32 if os.name == 'nt' else 1 FILE_CHAR_END = 128 -FILE_CHAR_BLOCKLIST = '/\\?%*:|"<>' if os.name == 'nt' else '/' +FILE_CHARS_DISALLOWED = '/\\?%*:|"<>' if os.name == 'nt' else '/' def notify_outputname(walletname, txid): @@ -29,7 +29,7 @@ class NotificationsTest(BitcoinTestFramework): self.setup_clean_chain = True def setup_network(self): - self.wallet = ''.join(chr(i) for i in range(FILE_CHAR_START, FILE_CHAR_END) if chr(i) not in FILE_CHAR_BLOCKLIST) + self.wallet = ''.join(chr(i) for i in range(FILE_CHAR_START, FILE_CHAR_END) if chr(i) not in FILE_CHARS_DISALLOWED) self.alertnotify_dir = os.path.join(self.options.tmpdir, "alertnotify") self.blocknotify_dir = os.path.join(self.options.tmpdir, "blocknotify") self.walletnotify_dir = os.path.join(self.options.tmpdir, "walletnotify") From 353a7b8a2ab1d9177167a3ddb6aae3fdb4b3085c Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Sun, 20 Sep 2020 11:13:49 +0200 Subject: [PATCH 05/19] Merge #19781: test: add parameterized constructor for msg_sendcmpct() 638441928a446726ce3a7fb20433a5478e7585bb test: add parameterized constructor for msg_sendcmpct() (Sebastian Falbesoner) Pull request description: While working on the test for #19776 I noticed that creating a `sendcmpct` message is quite cumbersome -- due to the lack of a parameterized constructor, one needs to create an empty (that is, initialized with default values) object and then set the two fields one by one. This PR replaces the default constructor with a parameterized constructor and uses it in the test `p2p_compactblocks.py`, reducing LOC. No need to pollute the namespace with temporary throw-away message objects anymore. ACKs for top commit: guggero: Code review ACK 638441928a446726ce3a7fb20433a5478e7585bb. epson121: Code review ACK 638441928a446726ce3a7fb20433a5478e7585bb Tree-SHA512: 3b58d276d714b73abc6cc98d1d52dec5f6026b33f03faaeb7dcbc5d83ac377555179f98b159b2b9ecc8957999c35a1dc082e3c69299c5fde4e35f1bd0587ce9d --- test/functional/p2p_compactblocks.py | 31 +++++----------------- test/functional/test_framework/messages.py | 6 ++--- 2 files changed, 10 insertions(+), 27 deletions(-) diff --git a/test/functional/p2p_compactblocks.py b/test/functional/p2p_compactblocks.py index cf4cb9135a..f06a746006 100755 --- a/test/functional/p2p_compactblocks.py +++ b/test/functional/p2p_compactblocks.py @@ -183,28 +183,21 @@ class CompactBlocksTest(BitcoinTestFramework): test_node.request_headers_and_sync(locator=[tip]) # Now try a SENDCMPCT message with too-high version - sendcmpct = msg_sendcmpct() - sendcmpct.version = preferred_version + 1 - sendcmpct.announce = True - test_node.send_and_ping(sendcmpct) + test_node.send_and_ping(msg_sendcmpct(announce=True, version=preferred_version+1)) check_announcement_of_new_block(node, test_node, lambda p: "cmpctblock" not in p.last_message) # Headers sync before next test. test_node.request_headers_and_sync(locator=[tip]) # Now try a SENDCMPCT message with valid version, but announce=False - sendcmpct.version = preferred_version - sendcmpct.announce = False - test_node.send_and_ping(sendcmpct) + test_node.send_and_ping(msg_sendcmpct(announce=False, version=preferred_version)) check_announcement_of_new_block(node, test_node, lambda p: "cmpctblock" not in p.last_message) # Headers sync before next test. test_node.request_headers_and_sync(locator=[tip]) # Finally, try a SENDCMPCT message with announce=True - sendcmpct.version = preferred_version - sendcmpct.announce = True - test_node.send_and_ping(sendcmpct) + test_node.send_and_ping(msg_sendcmpct(announce=True, version=preferred_version)) check_announcement_of_new_block(node, test_node, lambda p: "cmpctblock" in p.last_message) # Try one more time (no headers sync should be needed!) @@ -215,15 +208,11 @@ class CompactBlocksTest(BitcoinTestFramework): check_announcement_of_new_block(node, test_node, lambda p: "cmpctblock" in p.last_message) # Try one more time, after sending a version-1, announce=false message. - sendcmpct.version = preferred_version - 1 - sendcmpct.announce = False - test_node.send_and_ping(sendcmpct) + test_node.send_and_ping(msg_sendcmpct(announce=False, version=preferred_version-1)) check_announcement_of_new_block(node, test_node, lambda p: "cmpctblock" in p.last_message) # Now turn off announcements - sendcmpct.version = preferred_version - sendcmpct.announce = False - test_node.send_and_ping(sendcmpct) + test_node.send_and_ping(msg_sendcmpct(announce=False, version=preferred_version)) check_announcement_of_new_block(node, test_node, lambda p: "cmpctblock" not in p.last_message and "headers" in p.last_message) # This code should be enabled after increasing cmctblk version @@ -231,9 +220,7 @@ class CompactBlocksTest(BitcoinTestFramework): if to_validate and old_node is not None: # Verify that a peer using an older protocol version can receive # announcements from this node. - sendcmpct.version = preferred_version - 1 - sendcmpct.announce = True - old_node.send_and_ping(sendcmpct) + old_node.send_and_ping(msg_sendcmpct(announce=True, version=preferred_version-1)) # Header sync old_node.request_headers_and_sync(locator=[tip]) check_announcement_of_new_block(node, old_node, lambda p: "cmpctblock" in p.last_message) @@ -681,11 +668,7 @@ class CompactBlocksTest(BitcoinTestFramework): node = self.nodes[0] tip = node.getbestblockhash() peer.get_headers(locator=[int(tip, 16)], hashstop=0) - - msg = msg_sendcmpct() - msg.version = peer.cmpct_version - msg.announce = True - peer.send_and_ping(msg) + peer.send_and_ping(msg_sendcmpct(announce=True, version=peer.cmpct_version)) def test_compactblock_reconstruction_multiple_peers(self, stalling_peer, delivery_peer): node = self.nodes[0] diff --git a/test/functional/test_framework/messages.py b/test/functional/test_framework/messages.py index a79a3954ba..970221f6d2 100755 --- a/test/functional/test_framework/messages.py +++ b/test/functional/test_framework/messages.py @@ -1871,9 +1871,9 @@ class msg_sendcmpct: __slots__ = ("announce", "version") command = b"sendcmpct" - def __init__(self): - self.announce = False - self.version = 1 + def __init__(self, announce=False, version=1): + self.announce = announce + self.version = version def deserialize(self, f): self.announce = struct.unpack(" Date: Sun, 4 Oct 2020 15:39:22 +0200 Subject: [PATCH 06/19] Merge #19723: Ignore unknown messages before VERACK 675e55e01392971aa56bda56cb09498b466d0902 Ignore unknown messages before VERACK (Suhas Daftuar) Pull request description: This allows for feature negotiation to take place with messages between VERSION and VERACK in the future, without requiring additional software changes to specifically ignore messages for features that are unimplemented by our software. ACKs for top commit: sipa: utACK 675e55e01392971aa56bda56cb09498b466d0902 practicalswift: ACK 675e55e01392971aa56bda56cb09498b466d0902: patch looks correct MarcoFalke: ACK 675e55e01392971aa56bda56cb09498b466d0902 hebasto: ACK 675e55e01392971aa56bda56cb09498b466d0902, the offender peer will be eventually disconnected due to the timeout. Tree-SHA512: 8d2b1d8b9843f2ee26b2c30f7c5ff0bfcfbe3f46b32cd0369c48ece26624151091237e83ce3f18c6da004099026602cfab1642ac916db777f047d170b365c007 --- src/net_processing.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index f6001796eb..c3c0ba7ce6 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -2889,8 +2889,7 @@ void PeerLogicValidation::ProcessMessage( } if (!pfrom.fSuccessfullyConnected) { - // Must have a verack message before anything else - Misbehaving(pfrom.GetId(), 1, "non-verack message before version handshake"); + LogPrint(BCLog::NET, "Unsupported message \"%s\" prior to verack from peer=%d\n", SanitizeString(msg_type), pfrom.GetId()); return; } From 54bc4ddcfc1f38c5e01cfc3da03597475d5206d4 Mon Sep 17 00:00:00 2001 From: fanquake Date: Tue, 27 Oct 2020 15:19:05 +0800 Subject: [PATCH 07/19] Merge #20152: doc: Update wallet files in files.md defe48a51f4315f8cc607875a099981593c8cc39 doc: Update wallet files in files.md (Hennadii Stepanov) Pull request description: This PR is a #19077 follow up, and it addresses the [comment](https://github.com/bitcoin/bitcoin/pull/19077#discussion_r504805234): > If need to update, there are two corrections that could be made: > > * Line 69 "Wallets are Berkeley DB (BDB) databases" is no longer true > > * Line 76 "Wallet lock file" should say "BDB wallet lock file" ACKs for top commit: RiccardoMasutti: ACK defe48a meshcollider: ACK defe48a51f4315f8cc607875a099981593c8cc39 Tree-SHA512: 39939f86a9c7842bf06913998305dcbd6209585f1da0fe9c274bac0572eb8464e59176884dd9e2b91312f34efad40cdeb4085ec72c2a2c1b33d16b6ab505140c --- doc/files.md | 36 +++++++++++++++++++++++++----------- 1 file changed, 25 insertions(+), 11 deletions(-) diff --git a/doc/files.md b/doc/files.md index 99f94e7ae1..1e7629cc3b 100644 --- a/doc/files.md +++ b/doc/files.md @@ -8,6 +8,10 @@ - [Multi-wallet environment](#multi-wallet-environment) + - [Berkeley DB database based wallets](#berkeley-db-database-based-wallets) + + - [SQLite database based wallets](#sqlite-database-based-wallets) + - [GUI settings](#gui-settings) - [Legacy subdirectories and files](#legacy-subdirectories-and-files) @@ -69,26 +73,36 @@ Subdirectory | File(s) | Description ## Multi-wallet environment -Wallets are Berkeley DB (BDB) databases: +Wallets are Berkeley DB (BDB) or SQLite databases. -Subdirectory | File(s) | Description --------------|-------------------|------------ -`database/` | BDB logging files | Part of BDB environment; created at start and deleted on shutdown; a user *must keep it as safe* as personal wallet `wallet.dat` -`./` | `db.log` | BDB error file -`./` | `wallet.dat` | Personal wallet with keys and transactions. May be either a Berkeley DB or SQLite database file. -`./` | `.walletlock` | Wallet lock file -`./` | `wallet.dat-journal` | SQLite Rollback Journal file for `wallet.dat`. Usually created at start and deleted on shutdown. A user *must keep it as safe* as the `wallet.dat` file. - -1. Each user-defined wallet named "wallet_name" resides in `wallets/wallet_name/` subdirectory. +1. Each user-defined wallet named "wallet_name" resides in the `wallets/wallet_name/` subdirectory. 2. The default (unnamed) wallet resides in `wallets/` subdirectory; if the latter does not exist, the wallet resides in the data directory. -3. A wallet database path can be specified by `-wallet` option. +3. A wallet database path can be specified with the `-wallet` option. 4. `wallet.dat` files must not be shared across different node instances, as that can result in key-reuse and double-spends due the lack of synchronization between instances. 5. Any copy or backup of the wallet should be done through a `backupwallet` call in order to update and lock the wallet, preventing any file corruption caused by updates during the copy. + +### Berkeley DB database based wallets + +Subdirectory | File(s) | Description +-------------|-------------------|------------- +`database/` | BDB logging files | Part of BDB environment; created at start and deleted on shutdown; a user *must keep it as safe* as personal wallet `wallet.dat` +`./` | `db.log` | BDB error file +`./` | `wallet.dat` | Personal wallet (a BDB database) with keys and transactions +`./` | `.walletlock` | BDB wallet lock file + +### SQLite database based wallets + +Subdirectory | File | Description +-------------|----------------------|------------- +`./` | `wallet.dat` | Personal wallet (a SQLite database) with keys and transactions +`./` | `wallet.dat-journal` | SQLite Rollback Journal file for `wallet.dat`. Usually created at start and deleted on shutdown. A user *must keep it as safe* as the `wallet.dat` file. + + ## GUI settings `dash-qt` uses [`QSettings`](https://doc.qt.io/qt-5/qsettings.html) class; this implies platform-specific [locations where application settings are stored](https://doc.qt.io/qt-5/qsettings.html#locations-where-application-settings-are-stored). From 8746f52a51e7c6964bf687197bdc35079e9266d5 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Sat, 28 Nov 2020 18:06:39 +0100 Subject: [PATCH 08/19] Merge #20522: [test] Fix sync issue in disconnect_p2ps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 3ebde2143aa98af213872b98b474d904e55056f7 [test] Fix wait condition in disconnect_p2ps (Amiti Uttarwar) Pull request description: #19315 currently has a [test failure](https://cirrus-ci.com/task/4545582645641216) because of a race. `disconnect_p2ps` is intended to have a `wait_until` clause that prevents this race, but the conditional doesn't match since its comparing two different object types. `MY_SUBVERSION` is defined in messages.py as a byte string, but is compared to the value returned by the RPC. This PR simply converts types to ensure they match, which should prevent the race from occurring. HUGE PROPS TO jnewbery for discovering the issue 🔎 ACKs for top commit: jnewbery: ACK 3ebde2143aa98af213872b98b474d904e55056f7 glozow: Code review ACK https://github.com/bitcoin/bitcoin/pull/20522/commits/3ebde2143aa98af213872b98b474d904e55056f7 Tree-SHA512: ca096b80a3e4d757a645f38846d6dc89d6a3d35c3435513a72d278e305faddd4aff9e75a767941b51b2abbf59c82679bac1e9a0140d6f285efe3053e51bcc2a8 --- test/functional/test_framework/test_node.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py index 0e25e4654e..004a169e2b 100755 --- a/test/functional/test_framework/test_node.py +++ b/test/functional/test_framework/test_node.py @@ -542,7 +542,7 @@ class TestNode(): def num_connected_mininodes(self): """Return number of test framework p2p connections to the node.""" - return len([peer for peer in self.getpeerinfo() if peer['subver'] == MY_SUBVERSION]) + return len([peer for peer in self.getpeerinfo() if peer['subver'] == MY_SUBVERSION.decode("utf-8")]) def disconnect_p2ps(self): """Close all p2p connections to the node.""" From c4b1c8b274d33f805e511932cb8ec13f1502c5ce Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Sun, 6 Dec 2020 19:12:49 +0100 Subject: [PATCH 09/19] Merge #19893: test: Remove or explain syncwithvalidationinterfacequeue fa6af312277bb1b7e57d9b764d411c5b0873829f test: Document why syncwithvalidationinterfacequeue is needed in tests (MarcoFalke) fa135a13b8ddaa117bd090ec43a3eab3a95755c1 Revert "test: Add missing sync_all to wallet_balance test" (MarcoFalke) Pull request description: syncwithvalidationinterfacequeue is a hidden test-only RPC, so it should not be used when it is not needed. Thus, either remove it or explain why it is needed. ACKs for top commit: fjahr: Code review ACK fa6af312277bb1b7e57d9b764d411c5b0873829f Tree-SHA512: de30db4ab521184091ee5beeab02989138cf7cf05088f766a2fb106151b239310b63d5380cb79e2a072f72c5ae9513aecae8eb9c1c7be713771585c3cb04d63a --- test/functional/wallet_balance.py | 2 -- test/functional/wallet_importmulti.py | 2 +- test/functional/wallet_resendwallettransactions.py | 1 + 3 files changed, 2 insertions(+), 3 deletions(-) diff --git a/test/functional/wallet_balance.py b/test/functional/wallet_balance.py index b05fd0ba32..4af6086aa5 100755 --- a/test/functional/wallet_balance.py +++ b/test/functional/wallet_balance.py @@ -204,8 +204,6 @@ class WalletTest(BitcoinTestFramework): self.log.info('Put txs back into mempool of node 1 (not node 0)') self.nodes[0].invalidateblock(block_reorg) self.nodes[1].invalidateblock(block_reorg) - self.sync_blocks() - self.nodes[0].syncwithvalidationinterfacequeue() assert_equal(self.nodes[0].getbalance(minconf=0), 0) # wallet txs not in the mempool are untrusted self.nodes[0].generatetoaddress(1, ADDRESS_WATCHONLY) assert_equal(self.nodes[0].getbalance(minconf=0), 0) # wallet txs not in the mempool are untrusted diff --git a/test/functional/wallet_importmulti.py b/test/functional/wallet_importmulti.py index a5e75c6d1a..fb9e5c2509 100755 --- a/test/functional/wallet_importmulti.py +++ b/test/functional/wallet_importmulti.py @@ -63,7 +63,7 @@ class ImportMultiTest(BitcoinTestFramework): self.nodes[0].generate(1) self.nodes[1].generate(1) timestamp = self.nodes[1].getblock(self.nodes[1].getbestblockhash())['mediantime'] - self.nodes[1].syncwithvalidationinterfacequeue() + self.nodes[1].syncwithvalidationinterfacequeue() # Sync the timestamp to the wallet, so that importmulti works node0_address1 = self.nodes[0].getaddressinfo(self.nodes[0].getnewaddress()) diff --git a/test/functional/wallet_resendwallettransactions.py b/test/functional/wallet_resendwallettransactions.py index 6893b2308a..1a3c510ecc 100755 --- a/test/functional/wallet_resendwallettransactions.py +++ b/test/functional/wallet_resendwallettransactions.py @@ -52,6 +52,7 @@ class ResendWalletTransactionsTest(BitcoinTestFramework): block.solve() node.submitblock(ToHex(block)) + # Set correct m_best_block_time, which is used in ResendWalletTransactions node.syncwithvalidationinterfacequeue() # Transaction should not be rebroadcast within first 12 hours From 058df132c4f9db6d1fce337b299c170b5aab8934 Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Thu, 10 Dec 2020 12:00:08 +0100 Subject: [PATCH 10/19] Merge #20589: log: Clarify that failure to read/write fee_estimates.dat is non-fatal fa0d8359b351fd179a0a2f458671a4d7828c9a80 log: Clarify that failure to read fee_estimates.dat is non-fatal (MarcoFalke) faefa5db5f1d95b772873f4429e8a8fbb4e71cf3 log: Clarify that failure to write fee_estimates.dat is non-fatal (MarcoFalke) Pull request description: two minor logging fixups ACKs for top commit: practicalswift: ACK fa0d8359b351fd179a0a2f458671a4d7828c9a80: patch looks correct laanwj: Code review ACK fa0d8359b351fd179a0a2f458671a4d7828c9a80 Tree-SHA512: d1e7e595d3b4a5e497ee7ab70f3be5783dafec2726ef8e012db836c15e8e622022859a4472d6b516fe19d327737b25fdfb509cd9aeb022ca847b13c54e55800a --- src/policy/fees.cpp | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/policy/fees.cpp b/src/policy/fees.cpp index 6530448b41..65e07d5dd8 100644 --- a/src/policy/fees.cpp +++ b/src/policy/fees.cpp @@ -6,6 +6,8 @@ #include #include +#include +#include #include #include #include @@ -870,7 +872,7 @@ void CBlockPolicyEstimator::Flush() { fs::path est_filepath = GetDataDir() / FEE_ESTIMATES_FILENAME; CAutoFile est_file(fsbridge::fopen(est_filepath, "wb"), SER_DISK, CLIENT_VERSION); if (est_file.IsNull() || !Write(est_file)) { - LogPrintf("Failed to write fee estimates to %s\n", est_filepath.string()); + LogPrintf("Failed to write fee estimates to %s. Continue anyway.\n", est_filepath.string()); } } @@ -906,8 +908,9 @@ bool CBlockPolicyEstimator::Read(CAutoFile& filein) int nVersionRequired, nVersionThatWrote; unsigned int nFileBestSeenHeight; filein >> nVersionRequired >> nVersionThatWrote; - if (nVersionRequired > CLIENT_VERSION) - return error("CBlockPolicyEstimator::Read(): up-version (%d) fee estimate file", nVersionRequired); + if (nVersionRequired > CLIENT_VERSION) { + throw std::runtime_error(strprintf("up-version (%d) fee estimate file", nVersionRequired)); + } // Read fee estimates file into temporary variables so existing data // structures aren't corrupted if there is an exception. @@ -924,8 +927,9 @@ bool CBlockPolicyEstimator::Read(CAutoFile& filein) std::vector fileBuckets; filein >> fileBuckets; size_t numBuckets = fileBuckets.size(); - if (numBuckets <= 1 || numBuckets > 1000) + if (numBuckets <= 1 || numBuckets > 1000) { throw std::runtime_error("Corrupt estimates file. Must have between 2 and 1000 feerate buckets"); + } auto fileFeeStats{std::make_unique(buckets, bucketMap, MED_BLOCK_PERIODS, MED_DECAY, MED_SCALE)}; auto fileShortStats{std::make_unique(buckets, bucketMap, SHORT_BLOCK_PERIODS, SHORT_DECAY, SHORT_SCALE)}; From 6c660cf29a73f1845b5bb5769dabf8a5ccef00cf Mon Sep 17 00:00:00 2001 From: fanquake Date: Sun, 27 Dec 2020 17:37:25 +0800 Subject: [PATCH 11/19] Merge #20674: fuzz: Call SendMessages after ProcessMessage to increase coverage fa09f97beabafaaeb59fca710760578ff1f2e8d7 fuzz: Call SendMessages after ProcessMessage to increase coverage (MarcoFalke) Pull request description: ACKs for top commit: practicalswift: Tested ACK fa09f97beabafaaeb59fca710760578ff1f2e8d7 dhruv: tACK fa09f97 Crypt-iQ: cr ACK fa09f97beabafaaeb59fca710760578ff1f2e8d7 sipa: utACK fa09f97beabafaaeb59fca710760578ff1f2e8d7 Tree-SHA512: 87c52aa38f902c4f6c9c2380f486a3ab21edc0e21e48bb619cdb67cfd698154cc57b170eef31fc940c0bb2c878e155847de03fc6e4cd85bed25f10c4f80c747b --- src/test/fuzz/process_message.cpp | 4 ++++ src/test/fuzz/process_messages.cpp | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/src/test/fuzz/process_message.cpp b/src/test/fuzz/process_message.cpp index 86ab187a25..afb3b3432d 100644 --- a/src/test/fuzz/process_message.cpp +++ b/src/test/fuzz/process_message.cpp @@ -73,6 +73,10 @@ void fuzz_target(const std::vector& buffer, const std::string& LIMIT_TO g_setup->m_node.peer_logic->ProcessMessage(p2p_node, random_message_type, random_bytes_data_stream, GetTimeMillis(), Params(), std::atomic{false}); } catch (const std::ios_base::failure& e) { } + { + LOCK(p2p_node.cs_sendProcessing); + g_setup->m_node.peer_logic->SendMessages(&p2p_node); + } SyncWithValidationInterfaceQueue(); } diff --git a/src/test/fuzz/process_messages.cpp b/src/test/fuzz/process_messages.cpp index 5d506e01cb..d2bb561d17 100644 --- a/src/test/fuzz/process_messages.cpp +++ b/src/test/fuzz/process_messages.cpp @@ -72,6 +72,10 @@ FUZZ_TARGET_INIT(process_messages, initialize_process_messages) connman.ProcessMessagesOnce(random_node); } catch (const std::ios_base::failure&) { } + { + LOCK(random_node.cs_sendProcessing); + g_setup->m_node.peer_logic->SendMessages(&random_node); + } } connman.ClearTestNodes(); SyncWithValidationInterfaceQueue(); From 54839f24afc1ac4d090af8efe6e1897276a4bba5 Mon Sep 17 00:00:00 2001 From: fanquake Date: Mon, 28 Dec 2020 13:36:48 +0800 Subject: [PATCH 12/19] Merge #20771: refactor: Enable -Wswitch for FeeEstimateHorizon faccf8b1e1af293dfe9158d732718e7798a2fd89 refactor: Enable -Wswitch for FeeEstimateHorizon (MarcoFalke) Pull request description: This enables the `-Wswitch` compiler warning for `FeeEstimateHorizon` by removing the `default` case in `switch` statements. ACKs for top commit: practicalswift: cr ACK faccf8b1e1af293dfe9158d732718e7798a2fd89 jonatack: ACK faccf8b1e1af293dfe9158d732718e7798a2fd89 hebasto: ACK faccf8b1e1af293dfe9158d732718e7798a2fd89, I have reviewed the code and it looks OK, I agree it can be merged. Tree-SHA512: 63a8dff6e8dead149ec2fa8319e7ff41022c9534d423d3086fd8f22be073dc4915f74c7fe9139ee681a8204730cf58c80ef40c93fb33032d586e68b4f78f557d --- src/policy/fees.cpp | 35 ++++++++++++++--------------------- 1 file changed, 14 insertions(+), 21 deletions(-) diff --git a/src/policy/fees.cpp b/src/policy/fees.cpp index 65e07d5dd8..fd2b693ecb 100644 --- a/src/policy/fees.cpp +++ b/src/policy/fees.cpp @@ -12,21 +12,18 @@ #include #include -static const char* FEE_ESTIMATES_FILENAME="fee_estimates.dat"; +static const char* FEE_ESTIMATES_FILENAME = "fee_estimates.dat"; static constexpr double INF_FEERATE = 1e99; -std::string StringForFeeEstimateHorizon(FeeEstimateHorizon horizon) { - static const std::map horizon_strings = { - {FeeEstimateHorizon::SHORT_HALFLIFE, "short"}, - {FeeEstimateHorizon::MED_HALFLIFE, "medium"}, - {FeeEstimateHorizon::LONG_HALFLIFE, "long"}, - }; - auto horizon_string = horizon_strings.find(horizon); - - if (horizon_string == horizon_strings.end()) return "unknown"; - - return horizon_string->second; +std::string StringForFeeEstimateHorizon(FeeEstimateHorizon horizon) +{ + switch (horizon) { + case FeeEstimateHorizon::SHORT_HALFLIFE: return "short"; + case FeeEstimateHorizon::MED_HALFLIFE: return "medium"; + case FeeEstimateHorizon::LONG_HALFLIFE: return "long"; + } // no default case, so the compiler can warn about missing cases + assert(false); } /** * We will instantiate an instance of this class to track transactions that were @@ -642,7 +639,7 @@ CFeeRate CBlockPolicyEstimator::estimateFee(int confTarget) const CFeeRate CBlockPolicyEstimator::estimateRawFee(int confTarget, double successThreshold, FeeEstimateHorizon horizon, EstimationResult* result) const { - TxConfirmStats* stats; + TxConfirmStats* stats = nullptr; double sufficientTxs = SUFFICIENT_FEETXS; switch (horizon) { case FeeEstimateHorizon::SHORT_HALFLIFE: { @@ -658,10 +655,8 @@ CFeeRate CBlockPolicyEstimator::estimateRawFee(int confTarget, double successThr stats = longStats.get(); break; } - default: { - throw std::out_of_range("CBlockPolicyEstimator::estimateRawFee unknown FeeEstimateHorizon"); - } - } + } // no default case, so the compiler can warn about missing cases + assert(stats); LOCK(m_cs_fee_estimator); // Return failure if trying to analyze a target we're not tracking @@ -691,10 +686,8 @@ unsigned int CBlockPolicyEstimator::HighestTargetTracked(FeeEstimateHorizon hori case FeeEstimateHorizon::LONG_HALFLIFE: { return longStats->GetMaxConfirms(); } - default: { - throw std::out_of_range("CBlockPolicyEstimator::HighestTargetTracked unknown FeeEstimateHorizon"); - } - } + } // no default case, so the compiler can warn about missing cases + assert(false); } unsigned int CBlockPolicyEstimator::BlockSpan() const From af5c102d3701c48d06198ccdcc2236cd524bde99 Mon Sep 17 00:00:00 2001 From: Samuel Dobson Date: Wed, 20 Jan 2021 16:02:05 +1300 Subject: [PATCH 13/19] Merge #20952: wallet: Add BerkeleyDB version sanity check at init time ad57fb756b1c2df625790bd9c296ec28daa93740 wallet: Add BerkeleyDB version sanity check at init time (Wladimir J. van der Laan) Pull request description: Detect version conflicts between the run-time BerkeleyDB library and the one used during compilation. This is very unsafe (can result in anything from crashes to corruption) so shut down when one is detected. ACKs for top commit: decryp2kanon: utACK ad57fb7 achow101: ACK ad57fb756b1c2df625790bd9c296ec28daa93740 theStack: utACK ad57fb756b1c2df625790bd9c296ec28daa93740 meshcollider: Code review ACK ad57fb756b1c2df625790bd9c296ec28daa93740 Tree-SHA512: 99cd7d836bffbdeb3d4e14053f7139cc85a6d42e631a3f9a3058a848042446b364faee127500f5acb374616e6a61ab2bedebfac1ba9bc993b4d6227114c2a6c2 --- src/wallet/bdb.cpp | 17 +++++++++++++++++ src/wallet/bdb.h | 4 ++++ src/wallet/init.cpp | 5 +++++ 3 files changed, 26 insertions(+) diff --git a/src/wallet/bdb.cpp b/src/wallet/bdb.cpp index 8870a324ee..63716bcab4 100644 --- a/src/wallet/bdb.cpp +++ b/src/wallet/bdb.cpp @@ -723,6 +723,23 @@ bool BerkeleyBatch::TxnAbort() return (ret == 0); } +bool BerkeleyDatabaseSanityCheck() +{ + int major, minor; + DbEnv::version(&major, &minor, nullptr); + + /* If the major version differs, or the minor version of library is *older* + * than the header that was compiled against, flag an error. + */ + if (major != DB_VERSION_MAJOR || minor < DB_VERSION_MINOR) { + LogPrintf("BerkeleyDB database version conflict: header version is %d.%d, library version is %d.%d\n", + DB_VERSION_MAJOR, DB_VERSION_MINOR, major, minor); + return false; + } + + return true; +} + std::string BerkeleyDatabaseVersion() { return DbEnv::version(nullptr, nullptr, nullptr); diff --git a/src/wallet/bdb.h b/src/wallet/bdb.h index fa81d45725..f263d40f66 100644 --- a/src/wallet/bdb.h +++ b/src/wallet/bdb.h @@ -222,6 +222,10 @@ public: std::string BerkeleyDatabaseVersion(); +/** Perform sanity check of runtime BDB version versus linked BDB version. + */ +bool BerkeleyDatabaseSanityCheck(); + //! Return object giving access to Berkeley database at specified path. std::unique_ptr MakeBerkeleyDatabase(const fs::path& path, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error); diff --git a/src/wallet/init.cpp b/src/wallet/init.cpp index 686d867dfb..49d490fa7c 100644 --- a/src/wallet/init.cpp +++ b/src/wallet/init.cpp @@ -118,6 +118,11 @@ void WalletInit::AddWalletOptions(ArgsManager& argsman) const bool WalletInit::ParameterInteraction() const { +#ifdef USE_BDB + if (!BerkeleyDatabaseSanityCheck()) { + return InitError(Untranslated("A version conflict was detected between the run-time BerkeleyDB library and the one used during compilation.")); + } +#endif if (gArgs.GetBoolArg("-disablewallet", DEFAULT_DISABLE_WALLET)) { for (const std::string& wallet : gArgs.GetArgs("-wallet")) { LogPrintf("%s: parameter interaction: -disablewallet -> ignoring -wallet=%s\n", __func__, wallet); From 6ad61a0efee11a78609b2c3cca7472c735a1324f Mon Sep 17 00:00:00 2001 From: fanquake Date: Fri, 22 Jan 2021 18:23:54 +0800 Subject: [PATCH 14/19] Merge #20985: doc: add xorriso to macOS depends packages 5b41d84b3469484ff6a1f4eb7c890b6444a16091 doc: add xorriso to macOS depends packages (fanquake) Pull request description: This was missed in #20470. ACKs for top commit: hebasto: ACK 5b41d84b3469484ff6a1f4eb7c890b6444a16091, tested on Linux Mint 20.1 (x86_64). Tree-SHA512: bcfd8468a099c69175f8a9d295c1466764ab25d6a61121b28675a09c3e96f45b6309e1523d341f4cb21d0ddee4945f00ba060ba02da835f2f0db7e694fd6c44b --- depends/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/depends/README.md b/depends/README.md index 6d16031b72..f470dd737a 100644 --- a/depends/README.md +++ b/depends/README.md @@ -41,7 +41,7 @@ The paths are automatically configured and no other options are needed unless ta #### For macOS cross compilation - sudo apt-get install curl librsvg2-bin libtiff-tools bsdmainutils cmake imagemagick libcap-dev libz-dev libbz2-dev python3-setuptools libtinfo5 + sudo apt-get install curl librsvg2-bin libtiff-tools bsdmainutils cmake imagemagick libcap-dev libz-dev libbz2-dev python3-setuptools libtinfo5 xorriso Note: You must obtain the macOS SDK before proceeding with a cross-compile. Under the depends directory, create a subdirectory named `SDKs`. From 09cf1a91244026e3e61a1db19d3fd6eeccaedd72 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Sun, 31 Jan 2021 08:43:52 +0100 Subject: [PATCH 15/19] Merge #21037: fuzz: Avoid designated initialization (C++20) in fuzz tests dee2d6fbf9008d0e0667b3744d847192be6ef6e0 fuzz: Avoid designated initialization (C++20) in fuzz tests (practicalswift) Pull request description: Avoid designated initialization (C++20) in fuzz tests. Context: https://github.com/bitcoin/bitcoin/pull/20197#discussion_r565270556, https://github.com/bitcoin/bitcoin/pull/20936#discussion_r566708730 ACKs for top commit: MarcoFalke: cr ACK dee2d6fbf9008d0e0667b3744d847192be6ef6e0 dhruv: code review ACK dee2d6fbf9008d0e0667b3744d847192be6ef6e0 ajtowns: utACK dee2d6fbf9008d0e0667b3744d847192be6ef6e0 Tree-SHA512: 5940fab6e97a2b11dd3b1475d2cffa2840dc2e6ec34bd9f9df90f948709cab98fd1c513d5dd104816d33a525a6e9710b8715b02db941e35d84f92bc211f56d1d --- src/test/fuzz/util.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/fuzz/util.h b/src/test/fuzz/util.h index 0038fd27e4..765dfcffbf 100644 --- a/src/test/fuzz/util.h +++ b/src/test/fuzz/util.h @@ -255,8 +255,8 @@ inline CNetAddr ConsumeNetAddr(FuzzedDataProvider& fuzzed_data_provider) noexcep const Network network = fuzzed_data_provider.PickValueInArray({Network::NET_IPV4, Network::NET_IPV6, Network::NET_INTERNAL, Network::NET_ONION}); CNetAddr net_addr; if (network == Network::NET_IPV4) { - const in_addr v4_addr = { - .s_addr = fuzzed_data_provider.ConsumeIntegral()}; + in_addr v4_addr = {}; + v4_addr.s_addr = fuzzed_data_provider.ConsumeIntegral(); net_addr = CNetAddr{v4_addr}; } else if (network == Network::NET_IPV6) { if (fuzzed_data_provider.remaining_bytes() >= 16) { From f08a10230fd61867ecc6b97937ef6145a09493b8 Mon Sep 17 00:00:00 2001 From: fanquake Date: Tue, 2 Feb 2021 08:56:31 +0800 Subject: [PATCH 16/19] Merge #21051: Fix -Wmismatched-tags warnings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit b6aadcd5b4350a6ebcd57e88e7a0853cedf7c2fb build: Add -Werror=mismatched-tags (Hennadii Stepanov) 1485124291368c4a2ca8ea09c18e813f1dbabf5c Fix -Wmismatched-tags warnings (Hennadii Stepanov) Pull request description: Warnings were introduced in #20749: ``` ./validation.h:43:1: warning: class 'CCheckpointData' was previously declared as a struct; this is valid, but may result in linker errors under the Microsoft C++ ABI [-Wmismatched-tags] class CCheckpointData; ^ ./chainparams.h:24:8: note: previous use is here struct CCheckpointData { ^ ./validation.h:43:1: note: did you mean struct here? class CCheckpointData; ^~~~~ struct 1 warning generated. ``` This change fixes AppVeyor build: https://ci.appveyor.com/project/DrahtBot/bitcoin/builds/37547435 ACKs for top commit: glozow: utACK https://github.com/bitcoin/bitcoin/pull/21051/commits/b6aadcd5b4350a6ebcd57e88e7a0853cedf7c2fb 🚗 practicalswift: cr ACK b6aadcd5b4350a6ebcd57e88e7a0853cedf7c2fb: patch looks correct Tree-SHA512: 3ac887ebdbf9a1ae33c1fd5381b3b8d83388ad557ddeb55013acd42bb9752a5bd009e3a0eed52644a023a7a0dda1c159277981af82f58fb0abfe60b84e01bf29 --- configure.ac | 1 + src/validation.h | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/configure.ac b/configure.ac index 017e2e00cd..6d3e05e3ce 100644 --- a/configure.ac +++ b/configure.ac @@ -435,6 +435,7 @@ if test "x$enable_werror" = "xyes"; then AX_CHECK_COMPILE_FLAG([-Werror=suggest-override],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=suggest-override"],,[[$CXXFLAG_WERROR]], [AC_LANG_SOURCE([[struct A { virtual void f(); }; struct B : A { void f() final; };]])]) AX_CHECK_COMPILE_FLAG([-Werror=unreachable-code-loop-increment],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=unreachable-code-loop-increment"],,[[$CXXFLAG_WERROR]]) + AX_CHECK_COMPILE_FLAG([-Werror=mismatched-tags], [ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=mismatched-tags"], [], [$CXXFLAG_WERROR]) fi if test "x$CXXFLAGS_overridden" = "xno"; then diff --git a/src/validation.h b/src/validation.h index ccba0b5bec..0164b4ba98 100644 --- a/src/validation.h +++ b/src/validation.h @@ -47,7 +47,7 @@ class CBlockIndex; class CBlockTreeDB; class CBlockUndo; class CChainParams; -class CCheckpointData; +struct CCheckpointData; class CInv; class CConnman; class CScriptCheck; From 3a59c53cad5ba572a17d079aa1cda16a805677fd Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Thu, 11 Feb 2021 10:34:42 +0100 Subject: [PATCH 17/19] Merge #21023: fuzz: Disable shuffle when merge=1 fabeb5b9c7f678ab3bc24c1860f8514ac52bb56f fuzz: Disable shuffle when merge=1 (MarcoFalke) Pull request description: This should hopefully help make the deletion of fuzz inputs more deterministic. My tests (N=1) revealed that without this patch 7000 files differ (https://github.com/bitcoin-core/qa-assets/pull/44#issuecomment-768841467). With this patch, "only" 2000 files differ. ACKs for top commit: practicalswift: cr ACK fabeb5b9c7f678ab3bc24c1860f8514ac52bb56f: `-shuffle=0` and `-prefer_small=1` make sense Tree-SHA512: 21a701f52450d402a91dd6e0b33d564c63a9c3b919738eb9a80c24d48fc5b964088e325470738f39af0d595612c844acc7bf0941590cc2dc8c6f6ee4cb69c861 --- test/fuzz/test_runner.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/fuzz/test_runner.py b/test/fuzz/test_runner.py index e1d084cac7..44e7766e90 100755 --- a/test/fuzz/test_runner.py +++ b/test/fuzz/test_runner.py @@ -215,6 +215,8 @@ def merge_inputs(*, fuzz_pool, corpus, test_list, build_dir, merge_dir): args = [ os.path.join(build_dir, 'src', 'test', 'fuzz', 'fuzz'), '-merge=1', + '-shuffle=0', + '-prefer_small=1', '-use_value_profile=1', # Also done by oss-fuzz https://github.com/google/oss-fuzz/issues/1406#issuecomment-387790487 os.path.join(corpus, t), os.path.join(merge_dir, t), From 513bd84fa3f63b3f2e9850c9fa6116c2b84bbdf2 Mon Sep 17 00:00:00 2001 From: fanquake Date: Wed, 17 Feb 2021 08:06:47 +0800 Subject: [PATCH 18/19] Merge #21159: test: fix sign comparison warning in socket tests 9cc8e30125df14fe47e21e55ab3bf26f4d416565 test: fix sign comparison warning in socket tests (fanquake) Pull request description: This fixes: ```bash In file included from test/sock_tests.cpp:10: In file included from /usr/local/include/boost/test/unit_test.hpp:18: In file included from /usr/local/include/boost/test/test_tools.hpp:46: /usr/local/include/boost/test/tools/old/impl.hpp:107:17: warning: comparison of integers of different signs: 'const long' and 'const unsigned long' [-Wsign-compare] return left == right; ~~~~ ^ ~~~~~ ``` which was introduced in #20788. ACKs for top commit: practicalswift: cr ACK 9cc8e30125df14fe47e21e55ab3bf26f4d416565 vasild: ACK 9cc8e30125df14fe47e21e55ab3bf26f4d416565 Tree-SHA512: 7069a4fde5cec01be03f8477fe396e53658f170efbf1d9ef3339d553bb90a2be9f4acd6b348127b14cd2f91426e0cd1fc35d2d3c9f201cf748c0cf50f47e46a5 --- src/test/sock_tests.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/sock_tests.cpp b/src/test/sock_tests.cpp index cc0e6e7057..ed9780dfb5 100644 --- a/src/test/sock_tests.cpp +++ b/src/test/sock_tests.cpp @@ -95,7 +95,7 @@ static void CreateSocketPair(int s[2]) static void SendAndRecvMessage(const Sock& sender, const Sock& receiver) { const char* msg = "abcd"; - constexpr size_t msg_len = 4; + constexpr ssize_t msg_len = 4; char recv_buf[10]; BOOST_CHECK_EQUAL(sender.Send(msg, msg_len, 0), msg_len); From 401b55e5c458115d763c51835023da1404ce0c4d Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Mon, 1 Mar 2021 11:39:28 +0100 Subject: [PATCH 19/19] Merge #18466: rpc: fix invalid parameter error codes for {sign,verify}message RPCs a5cfb40e27bd281354bd0d14d91f83efb6bfce9f doc: release note for changed {sign,verify}message error codes (Sebastian Falbesoner) 9e399b9b2d386b28c0c0ff59fc75d31dbec31d9c test: check parameter validity in rpc_signmessage.py (Sebastian Falbesoner) e62f0c71f10def124b1c1219d790cef246a32c3e rpc: fix {sign,message}verify RPC errors for invalid address/signature (Sebastian Falbesoner) Pull request description: RPCs that accept address parameters usually return the intended error code `RPC_INVALID_ADDRESS_OR_KEY` (-5) if a passed address is invalid. The two exceptions to the rule are `signmessage` and `verifymessage`, which return `RPC_TYPE_ERROR` (-3) in this case instead. Oddly enough `verifymessage` returns `RPC_INVALID_ADDRESS_OR_KEY` when the _signature_ was malformed, where `RPC_TYPE_ERROR` would be more approriate. This PR fixes these inaccuracies and as well adds tests to `rpc_signmessage.py` that check the parameter validity and error codes for the related RPCs `signmessagewithprivkey`, `signmessage` and `verifymessage`. master branch: ``` $ ./bitcoin-cli signmessage invalid_addr message error code: -3 error message: Invalid address $ ./bitcoin-cli verifymessage invalid_addr dummy_sig message error code: -3 error message: Invalid address $ ./bitcoin-cli verifymessage 12c6DSiU4Rq3P4ZxziKxzrL5LmMBrzjrJX invalid_sig message error code: -5 error message: Malformed base64 encoding ``` PR branch: ``` $ ./bitcoin-cli signmessage invalid_addr message error code: -5 error message: Invalid address $ ./bitcoin-cli verifymessage invalid_addr dummy_sig message error code: -5 error message: Invalid address $ ./bitcoin-cli verifymessage 12c6DSiU4Rq3P4ZxziKxzrL5LmMBrzjrJX invalid_sig message error code: -3 error message: Malformed base64 encoding ``` ACKs for top commit: laanwj: Code review ACK a5cfb40e27bd281354bd0d14d91f83efb6bfce9f meshcollider: utACK a5cfb40e27bd281354bd0d14d91f83efb6bfce9f Tree-SHA512: bae0c4595a2603cea66090f6033785601837b45fd853052312b3a39d8520566c581994b68f693dd247c22586c638c3b7689c849085cce548cc36b9bf0e119d2d --- doc/release-notes-18466.md | 10 ++++++++++ src/rpc/misc.cpp | 4 ++-- src/wallet/rpcwallet.cpp | 2 +- test/functional/rpc_signmessage.py | 22 +++++++++++++++++++++- 4 files changed, 34 insertions(+), 4 deletions(-) create mode 100644 doc/release-notes-18466.md diff --git a/doc/release-notes-18466.md b/doc/release-notes-18466.md new file mode 100644 index 0000000000..e46d5064a3 --- /dev/null +++ b/doc/release-notes-18466.md @@ -0,0 +1,10 @@ +Low-level RPC changes +--------------------- + +- Error codes have been updated to be more accurate for the following error cases (#18466): + - `signmessage` now returns RPC_INVALID_ADDRESS_OR_KEY (-5) if the + passed address is invalid. Previously returned RPC_TYPE_ERROR (-3). + - `verifymessage` now returns RPC_INVALID_ADDRESS_OR_KEY (-5) if the + passed address is invalid. Previously returned RPC_TYPE_ERROR (-3). + - `verifymessage` now returns RPC_TYPE_ERROR (-3) if the passed signature + is malformed. Previously returned RPC_INVALID_ADDRESS_OR_KEY (-5). diff --git a/src/rpc/misc.cpp b/src/rpc/misc.cpp index 8776aa4908..d417c02187 100644 --- a/src/rpc/misc.cpp +++ b/src/rpc/misc.cpp @@ -447,11 +447,11 @@ static UniValue verifymessage(const JSONRPCRequest& request) switch (MessageVerify(strAddress, strSign, strMessage)) { case MessageVerificationResult::ERR_INVALID_ADDRESS: - throw JSONRPCError(RPC_TYPE_ERROR, "Invalid address"); + throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid address"); case MessageVerificationResult::ERR_ADDRESS_NO_KEY: throw JSONRPCError(RPC_TYPE_ERROR, "Address does not refer to key"); case MessageVerificationResult::ERR_MALFORMED_SIGNATURE: - throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Malformed base64 encoding"); + throw JSONRPCError(RPC_TYPE_ERROR, "Malformed base64 encoding"); case MessageVerificationResult::ERR_PUBKEY_NOT_RECOVERED: case MessageVerificationResult::ERR_NOT_SIGNED: return false; diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 6c68ce3e48..454b0a7fd8 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -589,7 +589,7 @@ static UniValue signmessage(const JSONRPCRequest& request) CTxDestination dest = DecodeDestination(strAddress); if (!IsValidDestination(dest)) { - throw JSONRPCError(RPC_TYPE_ERROR, "Invalid address"); + throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid address"); } const PKHash *pkhash = std::get_if(&dest); diff --git a/test/functional/rpc_signmessage.py b/test/functional/rpc_signmessage.py index bbe4aa03a5..9c1dab8893 100755 --- a/test/functional/rpc_signmessage.py +++ b/test/functional/rpc_signmessage.py @@ -5,7 +5,10 @@ """Test RPC commands for signing and verifying messages.""" from test_framework.test_framework import BitcoinTestFramework -from test_framework.util import assert_equal +from test_framework.util import ( + assert_equal, + assert_raises_rpc_error, +) class SignMessagesTest(BitcoinTestFramework): def set_test_params(self): @@ -37,5 +40,22 @@ class SignMessagesTest(BitcoinTestFramework): assert not self.nodes[0].verifymessage(other_address, signature, message) assert not self.nodes[0].verifymessage(address, other_signature, message) + self.log.info('test parameter validity and error codes') + # signmessage(withprivkey) have two required parameters + for num_params in [0, 1, 3, 4, 5]: + param_list = ["dummy"]*num_params + assert_raises_rpc_error(-1, "signmessagewithprivkey", self.nodes[0].signmessagewithprivkey, *param_list) + assert_raises_rpc_error(-1, "signmessage", self.nodes[0].signmessage, *param_list) + # verifymessage has three required parameters + for num_params in [0, 1, 2, 4, 5]: + param_list = ["dummy"]*num_params + assert_raises_rpc_error(-1, "verifymessage", self.nodes[0].verifymessage, *param_list) + # invalid key or address provided + assert_raises_rpc_error(-5, "Invalid private key", self.nodes[0].signmessagewithprivkey, "invalid_key", message) + assert_raises_rpc_error(-5, "Invalid address", self.nodes[0].signmessage, "invalid_addr", message) + assert_raises_rpc_error(-5, "Invalid address", self.nodes[0].verifymessage, "invalid_addr", signature, message) + # malformed signature provided + assert_raises_rpc_error(-3, "Malformed base64 encoding", self.nodes[0].verifymessage, self.nodes[0].getnewaddress(), "invalid_sig", message) + if __name__ == '__main__': SignMessagesTest().main()