[META] This is a followup to "validation: Move FindFilesToPrune{,Manual}
to BlockManager" removing comments and assertions meant only to
show that the change is correct.
[META] No behaviour change is intended in this commit.
[META] This commit should be followed up by removing the comments and
assertions meant only to show that the change is correct.
Also stop FindFilesToPrune{,Manual} from unnecessary reaching for
::ChainActive() by passing in the necessary information.
As discussed in https://github.com/bitcoin/bitcoin/issues/19943, this
test may be a bit misleading to newcomers.
We underscore the fact that our peer needs to run a modified version of
Bitcoin Core to actually relay transactions to a `blocksonly` node and
benefit from the `whitelistforcerelay` parameter.
Recognizing addresses from those networks allows us to accept and gossip
them, even though we don't know how to connect to them (yet).
Co-authored-by: eriknylund <erik@daychanged.com>
638441928a 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 638441928a.
epson121:
Code review ACK 638441928a
Tree-SHA512: 3b58d276d714b73abc6cc98d1d52dec5f6026b33f03faaeb7dcbc5d83ac377555179f98b159b2b9ecc8957999c35a1dc082e3c69299c5fde4e35f1bd0587ce9d
a06eb03ded doc: Add comments and additional reviewers to CODEOWNERS file (Adam Jonas)
e02da22906 doc: Add CODEOWNERS file (Wladimir J. van der Laan)
Pull request description:
This PR brings back and builds on #17094. GitHub uses a CODEOWNERS magic file to automatically add tagged contributors to the "Reviewers" list for a PR.
The goal of this is to make use of GitHub's suggested reviewers feature and not to confer ownership or give veto power to specific people. It would be better if this file could be named CODEREVIEWERS, but alas, that wouldn't work. The idea of a NAGFILE was proposed in [Bitcoin Core Dev meeting in 2018](https://diyhpl.us/wiki/transcripts/bitcoin-core-dev-tech/2018-03-07-priorities/#:~:text=NAGFILE). While this GitHub implementation has some complications, it's a step towards realizing the promise of automating "reviewing begging" and (hopefully) positively impacting the review process as a whole.
Of secondary value, this file can serve as documentation for who the maintainers are and who it might be smart to check with for certain areas of code/features (i.e., fuzzing, PSBT, and Bech32) -- this is helpful information for new contributors.
* The first commit is taken from #17094
* The second commit adds comments and expands the list of reviewers based on the suggestions and comments from that PR
* ~The third WIP commit~ This commit also uses the doc dir as an example of granular assignments based on lines of codes ~contributed~ written and/or general engagement with the project. (If interested, here is a report for [most lines of code per author for each file](https://gist.github.com/adamjonas/854a46a1918224927b186865baeac411)). The pro of this level of detail is that the best reviewer is more likely to be nominated. The con is that it will create churn as files are renamed, new files are added, or reviewers want to be added or removed.
Some open questions:
* How often should this file be changed?
* What level of history does one need have on the project before being added to this file? When does it make sense to remove a reviewer?
* These review notifications can [cause a lot of noise](https://github.community/t5/How-to-use-Git-and-GitHub/Team-based-notifications-or-rework-CODEOWNERS-notification/td-p/7811) and automatically subscribes the requested reviewer to the thread. A GitHub Team based approach would allow for adding or removing reviewers without modifying this file; however, this comes along with its [own set of problems](https://bionic.fullstory.com/taming-github-codeowners-with-bots/#problems-with-github-code-owners), including granting [write access](https://github.community/t5/How-to-use-Git-and-GitHub/CODEOWNERS-works-with-users-but-not-teams/td-p/4986#U4991). Other projects [have used bots](https://bionic.fullstory.com/taming-github-codeowners-with-bots/#using-a-github-bot) to sidestep this.
Top commit has no ACKs.
Tree-SHA512: aa674ac62478b8801f48750df869c802070dc83d0fa9ff93596e9d63406129d7fd3c0daeb35d7a1a259554d045c24746a6808878a7b9867c7ed66d251f0c918f
6fe2ef2acb scripted-diff: Rename SendMessage to SendZmqMessage. (Daniel Kraft)
a3ffb6ebeb Replace zmqconfig.h by a simple zmqutil. (Daniel Kraft)
7f2ad1b9ac Use std::unique_ptr for CZMQNotifierFactory. (Daniel Kraft)
b93b9d5456 Simplify and fix notifier removal on error. (Daniel Kraft)
e15b1cfc31 Various cleanups in zmqnotificationinterface. (Daniel Kraft)
Pull request description:
This contains various small code cleanups that make the ZMQ code easier to read and maintain (at least in my opinion). The only functional change is that a potential memory leak is fixed that would have occured when a notifier is removed from the `notifiers` list after its callback function returned `false` (which is likely not relevant in practice but still a bug).
ACKs for top commit:
instagibbs:
utACK 6fe2ef2acb
hebasto:
re-ACK 6fe2ef2acb, only the latest commit got a scripted-diff since my [previous](https://github.com/bitcoin/bitcoin/pull/13686#pullrequestreview-487649808) review.
Tree-SHA512: 8206f8713bf3698d7cd4cb235f6657dc1c4dd920f50a8c5f371a559dd17ce5ab6d94d6281165eef860a22fc844a6bb25489ada12c83ebc780efd7ccdc0860f70
23c35bf005 [test] add get_vsize util for more programmatic testing (gzhao408)
2233a93a10 [rpc] Return fee and vsize from testmempoolaccept (codeShark149)
Pull request description:
From #19093 and resolves#19057.
Difference from #19093: return `vsize` and `fees` object (similar to `getmempoolentry`) when the test accept is successful. Updates release-notes.md.
ACKs for top commit:
jnewbery:
utACK 23c35bf005
fjahr:
utACK 23c35bf
instagibbs:
reACK 23c35bf005
Tree-SHA512: dcb81b7b817a4684e9076bc5d427a6f2d549d2edc66544e718260c4b5f8f1d5ae1d47b754175e9f0c8a3bd8371ce116c2dca0583588d513a7d733d5d614f2b04
a5f5374b43 test: create default wallet in extended tests (Sjors Provoost)
Pull request description:
This was omitted from #15454
ACKs for top commit:
ryanofsky:
Code review ACK a5f5374b43. Just reverted a leftover diff since last review
gzhao408:
utACK a5f5374b43
Tree-SHA512: 573e215e3665cd23f58417a7ebf66a73420645450f8bc51a7bbb36dea6bfda838f6131bb4456aea35d9dac57b61741bba704a7df8ed11409c21fb8001ec55588
d26f0648f1 Tell users how to load or create a wallet when no wallet is loaded (Andrew Chow)
1bee1e6269 Do not create default wallet (Andrew Chow)
Pull request description:
Instead of automatically creating and loading a default wallet, users should instead explicitly create their wallet or load it on start.
Builds on #19754 which provides the `load_on_startup` behavior for the GUI.
ACKs for top commit:
jnewbery:
Manual test and very light code review ACK d26f0648f1
ryanofsky:
Code review ACK d26f0648f1. Just suggested changes to first commit (reusing MakeWalletDatabase and adding release notes), no changes to second commit
jonatack:
ACK d26f0648f1 light code review, debug build, ran tests, did manual testing with testnet, rebased on master, on linux debian.
Tree-SHA512: 091d785aef64736f7df661c576e815a87f3d029cfa32f3a75ba86fc25795f10b022ab3ae15c5b61a10b8cee16f5650f15cd79cbd6127e5e3ccbef631966d3c30
fc7f84a9ca tests: Add fuzzing harness for Keccak and SHA3_256 (practicalswift)
Pull request description:
Add fuzzing harness for Keccak and SHA3_256.
See [`doc/fuzzing.md`](https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md) for information on how to fuzz Bitcoin Core. Don't forget to contribute any coverage increasing inputs you find to the [Bitcoin Core fuzzing corpus repo](https://github.com/bitcoin-core/qa-assets).
Happy fuzzing :)
ACKs for top commit:
laanwj:
uACK fc7f84a9ca
elichai:
utACK :) fc7f84a9ca
Tree-SHA512: 01e1610e1c178d5f42578e2dd5644a4165596db34cf5037d574a5285e0ace4b06dc33ab81a308595246117537fe175294efd4bfc174ffc2e8eac98f0ec9dd3e9
e1fdd2963b Test batch rpc with params (Gregory Sanders)
Pull request description:
Useful as an example and test case.
ACKs for top commit:
laanwj:
ACK e1fdd2963b
theStack:
ACK e1fdd2963b
Tree-SHA512: 2d2ba8960916342b264a14624857d6dd10005be12efafb3e970b82656f721c8f3700ebc9b8809de1b2f887d482b772043504aeaeebc7f2e1c8203f076a451526
a8a64acaf3 [BroadcastTransaction] Remove unsafe move operator (Amiti Uttarwar)
125c038126 [p2p] Remove dead code (Amiti Uttarwar)
fc66d0a65c [p2p] Check for nullptr before dereferencing pointer (Adam Jonas)
cb79b9dbf4 [mempool] Revert unbroadcast set to tracking just txid (Amiti Uttarwar)
Pull request description:
Addresses some outstanding review comments from #18044
- reverts unbroadcast txids to a set instead of a map (simpler, communicates intent better, takes less space, no efficiency advantages of map)
- adds safety around two touchpoints (check for nullptr before dereferencing pointer, remove an inaccurate std::move operator)
- removes some dead code
Links to comments on wtxid PR: [1](https://github.com/bitcoin/bitcoin/pull/18044#discussion_r460495254) [2](https://github.com/bitcoin/bitcoin/pull/18044#discussion_r460496023) [3](https://github.com/bitcoin/bitcoin/pull/18044#discussion_r463532611)
thanks to jnewbery & adamjonas for flagging these ! !
ACKs for top commit:
sdaftuar:
utACK a8a64acaf3
naumenkogs:
utACK a8a64acaf3
jnewbery:
utACK a8a64acaf3
Tree-SHA512: 7be669cb30cc17fb9e06b50e636ef7887c6a27354697987e4e4d38dba4b8f50e175647587430cd9bc3295bec01ce8b1e6639a50a4249d8fff9b1ca1b9ead3277
[META] This is a pure refactor commit.
Move PruneBlockFile to BlockManager because:
1. PruneOneBlockFile only acts on BlockManager
2. Eliminates the need for callers (FindFilesToPrune{,Manual}) to have a
reference to the larger ChainstateManager, just a reference to
BlockManager is enough. See following commits.
This is an alternative to #19751 that fixes the build without requiring
splitting out libpng. This patch can be dropped once we are building qt
5.12.0 or later.
916d3596c4 help: Generate checkpoint height from chainparams (Luke Dashjr)
Pull request description:
Not sure if this is worth putting in Core, but might as well until checkpoints are removed entirely.
ACKs for top commit:
laanwj:
re-ACK 916d3596c4
Tree-SHA512: d8eb26b570ee730fdd75ca916507134db5f2f68987a911e33544b7f1c9ccfd1c76b9c9db63056971956b6daf16910f17ecfc197481c2f7b0773afdfbf7d381cf
fc9278d162 build: AX_PTHREAD serial 27 (fanquake)
15c27c4441 build: split PTHREAD_* flags out of AM_LDFLAGS (fanquake)
68e3e22944 scripted-diff: add FUZZ_SUITE_LDFLAGS_COMMON (fanquake)
afecde8046 build: add PTHREAD_LIBS to LDFLAGS configure output (fanquake)
Pull request description:
TLDR: Split pthread flags out of ldflags, and stop using them when building libconsensus.
Building libconsensus on Linux using Clang currently warns. i.e:
```bash
./autogen.sh
./configure --disable-tests --disable-bench --with-utils=no --with-daemon=no --with-gui=no --disable-wallet --with-libs=yes CC=clang CXX=clang++
make V=1 -j6
... -Wl,-z -Wl,relro -Wl,-z -Wl,now -pthread -Wl,-soname -Wl,libbitcoinconsensus.so.0 -o .libs/libbitcoinconsensus.so.0.0.0
clang: warning: argument unused during compilation: '-pthread' [-Wunused-command-line-argument]
clang: warning: argument unused during compilation: '-pthread' [-Wunused-command-line-argument]
```
Besides wanting to quiet the warnings, after digging into this it seemed we could clean up how we are passing around the pthread flags. I also learnt a bit more about how libtools builds shared libraries, and that passing `-pthread` on the link line wouldn't be enough to link against pthreads anyways, due to libtools usage of -nostdlib (see [related discussion where we build DLLs](476436b2de/configure.ac (L603))).
This can be demonstrated with a patch to libconsensus:
```patch
diff --git a/src/script/bitcoinconsensus.cpp b/src/script/bitcoinconsensus.cpp
index 15e204062..10bf3582f 100644
--- a/src/script/bitcoinconsensus.cpp
+++ b/src/script/bitcoinconsensus.cpp
@@ -10,6 +10,8 @@
#include <script/interpreter.h>
#include <version.h>
+#include <pthread.h>
+
namespace {
/** A class that deserializes a single CTransaction one time. */
@@ -127,3 +129,10 @@ unsigned int bitcoinconsensus_version()
// Just use the API version for now
return BITCOINCONSENSUS_API_VER;
}
+
+void *func_pthread(void *x) { return x; }
+
+void f() {
+ pthread_t t;
+ pthread_create(&t,0,func_pthread,0);
+}
```
After building, you'll find you have a `libbitcoinconsensus.so` using pthread symbols, but which isn't linked against libpthread:
```bash
ldd -r src/.libs/libbitcoinconsensus.so
linux-vdso.so.1 (0x00007ffe49378000)
libstdc++.so.6 => /usr/lib/x86_64-linux-gnu/libstdc++.so.6 (0x00007f553cee7000)
libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007f553cda2000)
libgcc_s.so.1 => /lib/x86_64-linux-gnu/libgcc_s.so.1 (0x00007f553cd88000)
libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007f553cbc5000)
/lib64/ld-linux-x86-64.so.2 (0x00007f553d15d000)
undefined symbol: pthread_create (src/.libs/libbitcoinconsensus.so)
```
This libtool behaviour has been known about for some time, i.e this [thread from 2005](https://gcc.gnu.org/bugzilla/show_bug.cgi?id=25460), describes the same issue. The suggestion from libtool maintainers at the time is to add `-lpthread` to LDFLAGS.
Also worth noting is that some of the users in those threads were also using the `AX_PTHREADS` macro, same as us, to determine how to compile with/link against pthreads. This macro has [recently been updated](https://git.savannah.gnu.org/gitweb/?p=autoconf-archive.git;a=commitdiff;h=2fb904589643eb6ca6122f834891b58d1d51b347), with reference to this issue. You can compare the output from the version we currently use, to the new version:
```bash
# our ax_pthread macro:
PTHREAD_CFLAGS = -pthread
PTHREAD_LIBS =
PTHREAD_CC = gcc / clang
# the new ax_pthread macro
PTHREAD_CFLAGS = -pthread
PTHREAD_LIBS = -lpthread
PTHREAD_CC = gcc / clang
```
Note that as part of this PR I've also added `PTHREAD_LIBS` to the split out flags. Although we weren't using it anywhere previously (and wouldn't have seemed to matter for the most part, given it was likely empty for most builders), the macro assumes it's use. i.e:
> NOTE: You are assumed to not only compile your program with these flags,
> but also to link with them as well. For example, you might link with
> $PTHREAD_CC $CFLAGS $PTHREAD_CFLAGS $LDFLAGS ... $PTHREAD_LIBS $LIBS
ACKs for top commit:
laanwj:
Code review ACK fc9278d162
hebasto:
re-ACK fc9278d162, only rebased and renamed s/`AM_PTHREAD_FLAGS`/`PTHREAD_FLAGS`/ since my [previous](https://github.com/bitcoin/bitcoin/pull/19558#pullrequestreview-473487730) review..
kallewoof:
ACK fc9278d162
Tree-SHA512: 7c0a5b0f0de4f54b1d7dce0e69020b341c37a383bb7c715867cc96c648774a557b1ddb42eb1b676f7bb2b822b69795bec14dc6272362d80662a21f10cb80331c
Windows headers define SendMessage as a macro, which leads to problems
with the method name "SendMessage". To circumvent this, we rename the
method to "SendZmqMessage".
-BEGIN VERIFY SCRIPT-
sed -i 's/SendMessage/SendZmqMessage/g' src/zmq/zmqpublishnotifier.*
-END VERIFY SCRIPT-
bf1f913c44 cli -netinfo: display multiple levels of details (Jon Atack)
077b3ac928 cli: change -netinfo optional arg from bool to int (Jon Atack)
4e2f2ddd64 cli: add getpeerinfo last_{block,transaction} to -netinfo (Jon Atack)
644be659ab cli: add -netinfo server version check and error message (Jon Atack)
ce57bf6cc0 cli: create peer connections report sorted by dir, minping (Jon Atack)
f5edd66e5d cli: create vector of Peer structs for peers data (Jon Atack)
3a0ab93e1c cli: add NetType enum struct and NetTypeEnumToString() (Jon Atack)
c227100919 cli: create local addresses, ports, and scores report (Jon Atack)
d3f77b736e cli: create inbound/outbound peer connections report (Jon Atack)
19377b2fd2 cli: start dashboard report with chain and version header (Jon Atack)
a3653c159e cli: tally peer connections by type (Jon Atack)
54799b66b4 cli: add ipv6 and onion address type detection helpers (Jon Atack)
12242b17a5 cli: create initial -netinfo option, NetinfoRequestHandler class (Jon Atack)
Pull request description:
This PR is inspired by laanwj's python script mentioned in #19405, which it turns out I ended up using every day and extending because I got hooked on using it to monitor Bitcoin peer connections.
For the full experience, run `./src/bitcoin-cli -netinfo 4`
On Linux, try it with watch `watch ./src/bitcoin-cli -netinfo 4`
Help doc
```
$ ./src/bitcoin-cli -help | grep -A3 netinfo
-netinfo
Get network peer connection information from the remote server. An
optional integer argument from 0 to 4 can be passed for different
peers listings (default: 0).
```
ACKs for top commit:
vasild:
ACK bf1f913
0xB10C:
ACK bf1f913c44
practicalswift:
ACK bf1f913c44 -- patch looks correct and is limited to `src/bitcoin-cli.cpp`
Tree-SHA512: b9d18e5cc2ffd2bb9f0295b5ac7609da8a9bbecaf823a26dfa706b5f07d5d1a8343081dad98b16aa9dc8efd8f41bc1a4acdc40259727de622dc7195ccf59c572
d11020019a Add OpenBSD instructions for building the Qt GUI (grubles)
Pull request description:
Using OpenBSD as a desktop OS is prevalent enough IMO to warrant updating the documentation for building the GUI.
ACKs for top commit:
fanquake:
ACK d11020019a - looks fine. Have not tested.
Tree-SHA512: a8078334fdd35438bcf87c3f5eae851c2a1ce961eb48ae50770bf2c556489da86b6ee198fe9fb732dcaddb2e0f2f4f55a3126971aae8f7d4e2e320dbb024e204
92326d8976 [rpc] add send method (Sjors Provoost)
2c2a1445dc [rpc] add snake case aliases for transaction methods (Sjors Provoost)
1bc8d0fd59 [rpc] walletcreatefundedpsbt: allow inputs to be null (Sjors Provoost)
Pull request description:
`walletcreatefundedpsbt` has some interesting features that `sendtoaddress` and `sendmany` don't have:
* manual coin selection
* outputting a PSBT (it was controversial to add this, see #18201)
* create a transaction without adding to wallet (which leads to broadcasting, unless `-walletbroadcast=0`)
At the same time `walletcreatefundedpsbt` can't broadcast a transaction, which is inconvenient for simple use cases.
This PR introduces a new `send` RPC method which creates a PSBT, signs it if possible and adds it to the wallet by default. If it can't sign all inputs, it outputs a PSBT. If `add_to_wallet` is set to `false` it will return the transaction in both PSBT and hex format.
Because it uses a PSBT internally, it will much easier to add hardware wallet support to this method (see #16546).
For `bitcoin-cli` users, it tries to keep the simplest use case easy to use:
```sh
bitcoin-cli -regtest send '{"ADDRESS": 0.1}' 1 sat/b
```
This paves the way for deprecating `sendtoaddress` and `sendmany` though there's no rush. The only missing feature compared to these older methods is adding labels to a destination address.
Depends on:
- [x] #16377 (`[rpc] don't automatically append inputs in walletcreatefundedpsbt`)
- [x] #11413 (`[wallet] [rpc] sendtoaddress/sendmany: Add explicit feerate option`)
- [x] #18244 (`[rpc] have lockUnspents also lock manually selected coins`)
ACKs for top commit:
meshcollider:
Light re-utACK 92326d8976
achow101:
ACK 92326d8976 Reviewed code and test, ran tests.
kallewoof:
utACK 92326d8976
Tree-SHA512: 7552ef1b193d4c06e381c44932fdb0d54f64383e4c7d6b988f49d059c7d4bba45ce6aa7813e03df86360ad9dad6f3010eb76ee7da480551742d5fd98c2251c0f
~CMainCleanup:
1. Is vestigial
2. References the g_chainman global (we should minimize g_chainman refs)
3. Only acts on g_chainman.m_blockman
4. Does the same thing as BlockManager::Unload
Note that with this change we are no-longer including PTHREAD_* flags
when building libbitcoinconsensus.
Also note that we are including PTHREAD_LIBS in AM_PTHREAD_FLAGS
