| Age | Commit message (Collapse) | Author |
|---|
|
916d3596c493fec44da86aeb92b61eafeea0b596 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 916d3596c493fec44da86aeb92b61eafeea0b596
Tree-SHA512: d8eb26b570ee730fdd75ca916507134db5f2f68987a911e33544b7f1c9ccfd1c76b9c9db63056971956b6daf16910f17ecfc197481c2f7b0773afdfbf7d381cf
|
|
building libconsensus
fc9278d162c342bace8a147da6bc4f9941d8d9d7 build: AX_PTHREAD serial 27 (fanquake)
15c27c44417ab77a660b53b8574f7eb5261b19f8 build: split PTHREAD_* flags out of AM_LDFLAGS (fanquake)
68e3e2294483cfee6bba8b5481eaee293e981ae8 scripted-diff: add FUZZ_SUITE_LDFLAGS_COMMON (fanquake)
afecde8046b5f13253f1a7d687b4a90841b5766c 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](https://github.com/bitcoin/bitcoin/blob/476436b2dec254bb988f8c7a6cbec1d7bb7cecfd/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 fc9278d162c342bace8a147da6bc4f9941d8d9d7
hebasto:
re-ACK fc9278d162c342bace8a147da6bc4f9941d8d9d7, 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 fc9278d162c342bace8a147da6bc4f9941d8d9d7
Tree-SHA512: 7c0a5b0f0de4f54b1d7dce0e69020b341c37a383bb7c715867cc96c648774a557b1ddb42eb1b676f7bb2b822b69795bec14dc6272362d80662a21f10cb80331c
|
|
bf1f913c4405cba35c8f99ec07b407940eb955b6 cli -netinfo: display multiple levels of details (Jon Atack)
077b3ac9284a90f33e31c64c49062ceaf815af60 cli: change -netinfo optional arg from bool to int (Jon Atack)
4e2f2ddd6444d93dd7e60ec67cc393dfc2a29b9d cli: add getpeerinfo last_{block,transaction} to -netinfo (Jon Atack)
644be659abfcf7c638e17795bafedad73bc55b27 cli: add -netinfo server version check and error message (Jon Atack)
ce57bf6cc0cdaf8233fd8a20e0d1c5b69d17d2a3 cli: create peer connections report sorted by dir, minping (Jon Atack)
f5edd66e5d136b229c805af9e6ea73218f6cedde cli: create vector of Peer structs for peers data (Jon Atack)
3a0ab93e1ce8d91235a6d46a57c6cb110fc5bf03 cli: add NetType enum struct and NetTypeEnumToString() (Jon Atack)
c227100919dd2422b29eb3bca9c0f1a7983cc3a8 cli: create local addresses, ports, and scores report (Jon Atack)
d3f77b736e43b187771b901a6a3452f83c116918 cli: create inbound/outbound peer connections report (Jon Atack)
19377b2fd24704bff6c805946575b116f07d5e0d cli: start dashboard report with chain and version header (Jon Atack)
a3653c159e4f5c887eec9ea608e474eaa299fc07 cli: tally peer connections by type (Jon Atack)
54799b66b466c0d015e6fe2f820663cc5d8e7998 cli: add ipv6 and onion address type detection helpers (Jon Atack)
12242b17a52c9833e6504f3f0a5b247a6e2fc5f9 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 bf1f913c4405cba35c8f99ec07b407940eb955b6
practicalswift:
ACK bf1f913c4405cba35c8f99ec07b407940eb955b6 -- patch looks correct and is limited to `src/bitcoin-cli.cpp`
Tree-SHA512: b9d18e5cc2ffd2bb9f0295b5ac7609da8a9bbecaf823a26dfa706b5f07d5d1a8343081dad98b16aa9dc8efd8f41bc1a4acdc40259727de622dc7195ccf59c572
|
|
92326d89766155a792254d30a9962251b8fc7799 [rpc] add send method (Sjors Provoost)
2c2a1445dc9d22c9d729b8301c8b3f54195bcfcf [rpc] add snake case aliases for transaction methods (Sjors Provoost)
1bc8d0fd5906bc9637d513cd193a1f47ad94da28 [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 92326d89766155a792254d30a9962251b8fc7799
achow101:
ACK 92326d89766155a792254d30a9962251b8fc7799 Reviewed code and test, ran tests.
kallewoof:
utACK 92326d89766155a792254d30a9962251b8fc7799
Tree-SHA512: 7552ef1b193d4c06e381c44932fdb0d54f64383e4c7d6b988f49d059c7d4bba45ce6aa7813e03df86360ad9dad6f3010eb76ee7da480551742d5fd98c2251c0f
|
|
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
|
|
-BEGIN VERIFY SCRIPT-
sed -i -e 's/\$(RELDFLAGS) \$(AM_LDFLAGS) \$(LIBTOOL_APP_LDFLAGS)$/\$(FUZZ_SUITE_LDFLAGS_COMMON)/' src/Makefile.test.include
patch -p1 << "EOF"
--- a/src/Makefile.test.include
+++ b/src/Makefile.test.include
@@ -323,6 +323,8 @@ endif
if ENABLE_FUZZ
+FUZZ_SUITE_LDFLAGS_COMMON = $(RELDFLAGS) $(AM_LDFLAGS) $(LIBTOOL_APP_LDFLAGS)
+
test_fuzz_addition_overflow_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES)
test_fuzz_addition_overflow_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS)
test_fuzz_addition_overflow_LDADD = $(FUZZ_SUITE_LD_COMMON)
EOF
-END VERIFY SCRIPT-
|
|
812037cb80f72096738cf2b0c15b39536c6c1e24 Change CSipHasher's count variable to uint8_t (Pieter Wuille)
Pull request description:
SipHash technically supports arbitrarily long inputs (at least, I couldn't find a limit in the [paper](https://eprint.iacr.org/2012/351.pdf)), but only the low 8 bits of the length matter. Because of that we should use an unsigned type to track the length (as any signed type could overflow, which is UB). `uint8_t` is sufficient, however.
Fixes #19930.
ACKs for top commit:
laanwj:
anyhow re-ACK 812037cb80f72096738cf2b0c15b39536c6c1e24
elichai:
utACK 812037cb80f72096738cf2b0c15b39536c6c1e24
practicalswift:
ACK 812037cb80f72096738cf2b0c15b39536c6c1e24
theStack:
ACK 812037cb80f72096738cf2b0c15b39536c6c1e24
Tree-SHA512: 5b1440c9e4591460da198991fb421ad47d2d96def2014e761726ce361aa9575752f2c4085656e7e9badee3660ff005cc76fbd1afe4848faefe4502f3412bd896
|
|
b9c1a7648131c5deec9704ee9acd00ec1820b9ce Squashed 'src/secp256k1/' changes from 2ed54da18a..8ab24e8dad (Pieter Wuille)
Pull request description:
This updates our src/secp256k1 subtree to the latest libsecp256k1 upstream version.
As it adds BIP340 support (see https://github.com/bitcoin-core/secp256k1/pull/558), this is a prerequisite for #17977. In particular, it contains:
* A few generic library improvements
* Support for x-only public keys as used by BIP340.
* Support for "key pair" objects, making signing more efficient by using a precomputed public key.
* Signing support for BIP340 Schnorr (single-party) signatures.
* Verification support for BIP340 Schnorr signatures.
* Support for verifying tweaked x-only keys, as used by BIP341's Taproot construction.
Things that are not included:
* MuSig, nor any kind of multisignatures, threshold signatures, ... on top.
* Batch verification.
* Support for variable-length messages in BIP340 (which are still being discussed, but won't affect BIP341, or Bitcoin Core).
* A few more generic improvements that are still in the pipeline, including faster modular inversions.
ACKs for top commit:
instagibbs:
ACK 894fb33f4c1b24667891f7d2aff9f486177b1173
fanquake:
ACK 894fb33f4c1b24667891f7d2aff9f486177b1173. Any Valgrind concerns will be addressed upstream, see discussion in https://github.com/bitcoin-core/secp256k1/pull/813, and if necessary, can be pulled into our tree prior to the 0.21.0 branch off. They are not a blocker for merging this PR in it's current state.
benthecarman:
ACK `894fb33`
Tree-SHA512: 6dc992f4477069b7fbd223316f1be955750923be1479c38adad2312649fdca1f316edb375c42ef9d97cea2407caaef49fb8c93abd6c037fe1a522910cbbc2479
|
|
8b39a875581bed1c2f40a7d9616bdb7cc642bf59 bugfix: make LoadWallet assigns status always (Akio Nakamura)
Pull request description:
In my enviroment, ```test/functional/wallet_multiwallet.py``` failed in line 237 for master( 147d50d63 ).
It got an expected rpc-error-message, but error code was not (-4) but (-18).
This is because that although loadwallet() in rpcwallet.cpp assumes LoadWallet() always assign some value to the 'status', but LoadWallet() does not do so in some situation.
This PR intends to fix above and prevends loadwallet() returns ambiguous error code.
ACKs for top commit:
hebasto:
re-ACK 8b39a875581bed1c2f40a7d9616bdb7cc642bf59, that is the same as 1728059730abef04f3fa84de0b6e20044be7a9d6.
ryanofsky:
Code review ACK 8b39a875581bed1c2f40a7d9616bdb7cc642bf59 (same as previous)
meshcollider:
utACK 8b39a875581bed1c2f40a7d9616bdb7cc642bf59
Tree-SHA512: a75d8240f60325bfdb69a07d392269fec97de743f38fe108371eb63a0aba5d8ce3cc484ecc69e81febf8040f5ab64f3a9450b98f8e07a0c17803784bb6f342bf
|
|
|
|
8ab24e8dad Merge #558: Add schnorrsig module which implements BIP-340 compliant signatures
f3733c5433 Merge #797: Fix Jacobi benchmarks and other benchmark improvements
cb5524adc5 Add benchmark for secp256k1_ge_set_gej_var
5c6af60ec5 Make jacobi benchmarks vary inputs
d0fdd5f009 Randomize the Z coordinates in bench_internal
c7a3424c5f Rename bench_internal variables
875d68b95f Merge #699: Initialize field elements when resulting in infinity
54caf2e74f Merge #799: Add fallback LE/BE for architectures with known endianness + SHA256 selftest
f431b3f28a valgrind_ctime_test: Add schnorrsig_sign
16ffa9d97c schnorrsig: Add taproot test case
8dfd53ee3f schnorrsig: Add benchmark for sign and verify
4e43520026 schnorrsig: Add BIP-340 compatible signing and verification
7332d2db6b schnorrsig: Add BIP-340 nonce function
7a703fd97d schnorrsig: Init empty experimental module
eabd9bc46a Allow initializing tagged sha256
6fcb5b845d extrakeys: Add keypair_xonly_tweak_add
58254463f9 extrakeys: Add keypair struct with create, pub and pub_xonly
f0010349b8 Separate helper functions for pubkey_create and seckey_tweak_add
910d9c284c extrakeys: Add xonly_pubkey_tweak_add & xonly_pubkey_tweak_add_test
176bfb1110 Separate helper function for ec_pubkey_tweak_add
4cd2ee474d extrakeys: Add xonly_pubkey with serialize, parse and from_pubkey
f49c9896b0 Merge #806: Trivial: Add test logs to gitignore
aabf00c155 Merge #648: Prevent ints from wrapping around in scratch space functions
f5adab16a9 Merge #805: Remove the extremely outdated TODO file.
bceefd6547 Add test logs to gitignore
1c325199d5 Remove the extremely outdated TODO file.
47e6618e11 extrakeys: Init empty experimental module
3e08b02e2a Make the secp256k1_declassify argument constant
8bc6aeffa9 Add SHA256 selftest
670cdd3f8b Merge #798: Check assumptions on integer implementation at compile time
5e5fb28b4a Use additional system macros to figure out endianness
7c068998ba Compile-time check assumptions on integer types
02b6c87b52 Add support for (signed) __int128
979961c506 Merge #787: Use preprocessor macros instead of autoconf to detect endianness
887bd1f8b6 Merge #793: Make scalar/field choice depend on C-detected __int128 availability
0dccf98a21 Use preprocessor macros instead of autoconf to detect endianness
b2c8c42cf1 Merge #795: Avoid linking libcrypto in the valgrind ct test.
57d3a3c64c Avoid linking libcrypto in the valgrind ct test.
79f1f7a4f1 Autodetect __int128 availability on the C side
0d7727f95e Add SECP256K1_FE_STORAGE_CONST_GET to 5x52 field
805082de11 Merge #696: Run a Travis test on s390x (big endian)
39295362cf Test travis s390x (big endian)
6034a04fb1 Merge #778: secp256k1_gej_double_nonzero supports infinity
f60915906d Merge #779: travis: Fix argument quoting for ./configure
9e49a9b255 travis: Fix argument quoting for ./configure
18d36327fd secp256k1_gej_double_nonzero supports infinity
214cb3c321 Merge #772: Improve constant-timeness on PowerPC
40412b1930 Merge #774: tests: Abort if malloc() fails during context cloning tests
2e1b9e0458 tests: Abort if malloc() fails during context cloning tests
67a429f31f Suppress a harmless variable-time optimization by clang in _int_cmov
5b196338f0 Remove redundant "? 1 : 0" after comparisons in scalar code
3e5cfc5c73 Merge #741: Remove unnecessary sign variable from wnaf_const
66bb9320c0 Merge #773: Fix some compile problems on weird/old compilers.
1309c03c45 Fix some compile problems on weird/old compilers.
2309c7dd4a Merge #769: Undef HAVE___INT128 in basic-config.h to fix gen_context compilation
22e578bb11 Undef HAVE___INT128 in basic-config.h to fix gen_context compilation
3f4a5a10e4 Merge #765: remove dead store in ecdsa_signature_parse_der_lax
f00d6575ca remove dead store in ecdsa_signature_parse_der_lax
dbd41db16a Merge #759: Fix uninitialized variables in ecmult_multi test
2e7fc5b537 Fix uninitialized variables in ecmult_multi test
37dba329c6 Remove unnecessary sign variable from wnaf_const
6bb0b77e15 Fix test_constant_wnaf for -1 and add a test for it.
47a7b8382f Clear field elements when writing infinity
61d1ecb028 Added test with additions resulting in infinity
60f7f2de5d Don't assume that ALIGNMENT > 1 in tests
ada6361dec Use ROUND_TO_ALIGN in scratch_create
8ecc6ce50e Add check preventing rounding to alignment from wrapping around in scratch_alloc
4edaf06fb0 Add check preventing integer multiplication wrapping around in scratch_max_allocation
git-subtree-dir: src/secp256k1
git-subtree-split: 8ab24e8dad9d43fc6661842149899e3cc9213b24
|
|
|
|
ab654c7d587b33d62230394663020439f80cee28 Unroll Keccak-f implementation (Pieter Wuille)
3f01ddb01bfffd49dfa131898d1c674ac5d0ac99 Add SHA3 benchmark (Pieter Wuille)
2ac8bf95834c8a43ebf365f09fb610829733134b Implement keccak-f[1600] and SHA3-256 (Pieter Wuille)
Pull request description:
Add a simple (and initially unoptimized) Keccak/SHA3 implementation based on https://github.com/mjosaarinen/tiny_sha3/blob/master/sha3.c, as one will be needed for TORv3 support (the conversion from BIP155 encoding to .onion notation uses a SHA3-based checksum). In follow-up commits, a benchmark is added, and the Keccakf function is unrolled for a (for me) 4.9x speedup.
Test vectors are taken from https://csrc.nist.gov/projects/cryptographic-algorithm-validation-program/secure-hashing#sha3vsha3vss.
ACKs for top commit:
practicalswift:
ACK ab654c7d587b33d62230394663020439f80cee28 -- patch looks correct and no sanitizer complaints when doing some basic fuzz testing of the added code (remember: **don't trust: fuzz!**) :)
laanwj:
re-ACK ab654c7d587b33d62230394663020439f80cee28
vasild:
ACK ab654c7
Tree-SHA512: 8a91b18c46e8fb178b7ff82046cff626180362337e515b92fbbd771876e795da2ed4e3995eb4849773040287f6e687237f469a90474ac53f521fc12e0f5031d9
|
|
|
|
Although loadwallet() in rpcwallet.cpp assumes LoadWallet() always
assign some value to the 'status', but LoadWallet() does not do so
in some situation.
This fixes above and prevends loadwallet() returns ambiguous error code.
|
|
|
|
|
|
This is of neglible use here, but it allows new RPC methods to take outputs as their first argument and make inputs optional.
|
|
bb6a32ce9983c72afa90f41a43a47ffd703ca006 [net processing] Move Misbehaving() to PeerManager (John Newbery)
aa114b1c9b06c2bd3ed936bbb9fb32b31f75bdb2 [net_processing] Move SendBlockTransactions into PeerManager (John Newbery)
3115e00f75b41d9765dcbb376e367b25f61a1d58 [net processing] Move MaybePunishPeerForTx to PeerManager (John Newbery)
e662e2d42afaf9c67c898634a0f3bc200255b6ea [net processing] Move ProcessOrphanTx to PeerManager (John Newbery)
b70cd890e375e904b7f36b3d959e5656f5a5cbcd [net processing] Move MaybePunishNodeForBlock into PeerManager (John Newbery)
d7778351bf60a21925a97b7fc4e9df5541b6d995 [net processing] Move ProcessHeadersMessage to PeerManager (John Newbery)
64f6162651420be2f4aa1498f0378a86780bc089 [whitespace] tidy up indentation after scripted diff (John Newbery)
58bd369b0ddd3383f7bdf7840912d18b96545f91 scripted-diff: [net processing] Rename PeerLogicValidation to PeerManager (John Newbery)
2297b26b3ce95e935c0ebb8c38dabf19965054a5 [net_processing] Pass chainparams to PeerLogicValidation constructor (John Newbery)
824bbd1ffba3df7ffa6f5bfaa31298cd484473b1 [move only] Collect all private members of PeerLogicValidation together (John Newbery)
Pull request description:
Continues the work of moving net_processing logic into PeerLogicValidation. See https://github.com/bitcoin/bitcoin/pull/19704 and https://github.com/bitcoin/bitcoin/pull/19607#discussion_r462032894 for motivation.
This PR also renames `PeerLogicValidation` to `PeerManager` as suggested in https://github.com/bitcoin/bitcoin/pull/10756#pullrequestreview-53892618.
ACKs for top commit:
MarcoFalke:
re-ACK bb6a32ce99 only change is rebase due to conflict in struct NodeContext and variable rename 🤸
hebasto:
re-ACK bb6a32ce9983c72afa90f41a43a47ffd703ca006, only rebased, and added renaming `s/peer_logic/peerman/` into scripted-diff since my [previous](https://github.com/bitcoin/bitcoin/pull/19791#pullrequestreview-483118079) review (verified with `git range-diff`).
Tree-SHA512: a2de4a521688fd25125b401e5575402c52b328a0fa27b3010567008d4f596b960aabbd02b2d81f42658f88f4365443fadb1008150a62fbcea123fb42d85a2c21
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
-BEGIN VERIFY SCRIPT-
sed -i 's/PeerLogicValidation/PeerManager/g' $(git grep -l PeerLogicValidation ./src ./test)
sed -i 's/peer_logic/peerman/g' $(git grep -l peer_logic ./src ./test)
-END VERIFY SCRIPT-
PeerLogicValidation was originally net_processing's implementation to
the validation interface. It has since grown to contain much of
net_processing's logic. Therefore rename it to reflect its
responsibilities.
Suggested in
https://github.com/bitcoin/bitcoin/pull/10756#pullrequestreview-53892618.
|
|
Keep a references to chainparams, rather than calling the global
Params() function every time it's needed. This is fine, since
globalChainParams does not get updated once it's been set, and it's
available at the point of constructing the PeerLogicValidation object.
|
|
We don't have a project style for ordering class members, but it always
makes sense to have no more than one of each public/protected/private
specifier.
Also move documentation for MaybeDiscourageAndDisconnect to the header.
|
|
296be8f58e02b39a58f017c52294aceed22c3ffd Get rid of unused functions CTxMemPool::GetMemPoolChildren, CTxMemPool::GetMemPoolParents (Jeremy Rubin)
46d955d196043cc297834baeebce31ff778dff80 Remove mapLinks in favor of entry inlined structs with iterator type erasure (Jeremy Rubin)
Pull request description:
Currently we have a peculiar data structure in the mempool called maplinks. Maplinks job is to track the in-pool children and parents of each transaction. This PR can be primarily understood and reviewed as a simple refactoring to remove this extra data structure, although it comes with a nice memory and performance improvement for free.
Maplinks is particularly peculiar because removing it is not as simple as just moving it's inner structure to the owning CTxMempoolEntry. Because TxLinks (the class storing the setEntries for parents and children) store txiters to each entry in the mempool corresponding to the parent or child, it means that the TxLinks type is "aware" of the boost multiindex (mapTx) it's coming from, which is in turn, aware of the entry type stored in mapTx. Thus we used maplinks to store this entry associated data we in an entirely separate data structure just to avoid a circular type reference caused by storing a txiter inside a CTxMempoolEntry.
It turns out, we can kill this circular reference by making use of iterator_to multiindex function and std::reference_wrapper. This allows us to get rid of the maplinks data structure and move the ownership of the parents/child sets to the entries themselves.
The benefit of this good all around, for any of the reasons given below the change would be acceptable, and it doesn't make the code harder to reason about or worse in any respect (as far as I can tell, there's no tradeoff).
### Simpler ownership model
No longer having to consistency check that mapLinks did have records for our CTxMempoolEntry, impossible to have a mapLinks entry outlive or incorrectly die before a CTxMempoolEntry.
### Memory Usage
We get rid of a O(Transactions) sized map in the mempool, which is a long lived data structure.
### Performance
If you have a CTxMemPoolEntry, you immediately know the address of it's children/parents, rather than having to do a O(log(Transactions)) lookup via maplinks (which we do very often). We do it in *so many* places that a true benchmark has to look at a full running node, but it is easy enough to show an improvement in this case.
The ComplexMemPool shows a good coherence check that we see the expected result of it being 12.5% faster / 1.14x faster.
```
Before:
# Benchmark, evals, iterations, total, min, max, median
ComplexMemPool, 5, 1, 1.40462, 0.277222, 0.285339, 0.279793
After:
# Benchmark, evals, iterations, total, min, max, median
ComplexMemPool, 5, 1, 1.22586, 0.243831, 0.247076, 0.244596
```
The ComplexMemPool benchmark only checks doing addUnchecked and TrimToSize for 800 transactions. While this bench does a good job of hammering the relevant types of function, it doesn't test everything.
Subbing in 5000 transactions shows a that the advantage isn't completely wiped out by other asymptotic factors (this isn't the only bottleneck in growing the mempool), but it's only a bit proportionally slower (10.8%, 1.12x), which adds evidence that this will be a good change for performance minded users.
```
# Benchmark, evals, iterations, total, min, max, median
ComplexMemPool, 5, 1, 59.1321, 11.5919, 12.235, 11.7068
# Benchmark, evals, iterations, total, min, max, median
ComplexMemPool, 5, 1, 52.1307, 10.2641, 10.5206, 10.4306
```
I don't think it's possible to come up with an example of where a maplinks based design would have better performance, but it's something for reviewers to consider.
# Discussion
## Why maplinks in the first place?
I spoke with the author of mapLinks (sdaftuar) a while back, and my recollection from our conversation was that it was implemented because he did not know how to resolve the circular dependency at the time, and there was no other reason for making it a separate map.
## Is iterator_to weird?
iterator_to is expressly for this purpose, see https://www.boost.org/doc/libs/1_51_0/libs/multi_index/doc/tutorial/indices.html#iterator_to
> iterator_to provides a way to retrieve an iterator to an element from a pointer to the element, thus making iterators and pointers interchangeable for the purposes of element pointing (not so for traversal) in many situations. This notwithstanding, it is not the aim of iterator_to to promote the usage of pointers as substitutes for real iterators: the latter are specifically designed for handling the elements of a container, and not only benefit from the iterator orientation of container interfaces, but are also capable of exposing many more programming bugs than raw pointers, both at compile and run time. iterator_to is thus meant to be used in scenarios where access via iterators is not suitable or desireable:
>
> - Interoperability with preexisting APIs based on pointers or references.
> - Publication of pointer-based interfaces (for instance, when designing a C-compatible library).
> - The exposure of pointers in place of iterators can act as a type erasure barrier effectively decoupling the user of the code from the implementation detail of which particular container is being used. Similar techniques, like the famous Pimpl idiom, are used in large projects to reduce dependencies and build times.
> - Self-referencing contexts where an element acts upon its owner container and no iterator to itself is available.
In other words, iterator_to is the perfect tool for the job by the last reason given. Under the hood it should just be a simple pointer cast and have no major runtime overhead (depending on if the function call is inlined).
Edit by laanwj: removed at sign from the description
ACKs for top commit:
jonatack:
re-ACK 296be8f per `git range-diff ab338a19 3ba1665 296be8f`, sanity check gcc 10.2 debug build is clean.
hebasto:
re-ACK 296be8f58e02b39a58f017c52294aceed22c3ffd, only rebased since my [previous](https://github.com/bitcoin/bitcoin/pull/19478#pullrequestreview-482400727) review (verified with `git range-diff`).
Tree-SHA512: f5c30a4936fcde6ae32a02823c303b3568a747c2681d11f87df88a149f984a6d3b4c81f391859afbeb68864ef7f6a3d8779f74a58e3de701b3d51f78e498682e
|
|
fafb381af8279b2d2ca768df0bf68d7eb036a2f9 Remove mempool global (MarcoFalke)
fa0359c5b30730744aa8a7cd9ffab79ded91041f Remove mempool global from p2p (MarcoFalke)
eeee1104d78eb59a582ee1709ff4ac2c33ee1190 Remove mempool global from init (MarcoFalke)
Pull request description:
This refactor unlocks some nice potential features, such as, but not limited to:
* Removing the fee estimates global (would avoid slightly fragile workarounds such as #18766)
* Making the mempool optional for a "blocksonly" operation mode
Even absent those features, the new code without the global should be easier to maintain, read and write tests for.
ACKs for top commit:
jnewbery:
utACK fafb381af8279b2d2ca768df0bf68d7eb036a2f9
hebasto:
ACK fafb381af8279b2d2ca768df0bf68d7eb036a2f9, I have reviewed the code and it looks OK, I agree it can be merged.
darosior:
ACK fafb381af8279b2d2ca768df0bf68d7eb036a2f9
Tree-SHA512: a2e696dc377e2e81eaf9c389e6d13dde4a48d81f3538df88f4da502d3012dd61078495140ab5a5854f360a06249fe0e1f6a094c4e006d8b5cc2552a946becf26
|
|
abac4367607d8d2b628e4db6a9663c960bacdacc wallet: Avoid multiple BerkeleyBatch in DelAddressBook (João Barbosa)
Pull request description:
ACKs for top commit:
achow101:
ACK abac4367607d8d2b628e4db6a9663c960bacdacc
jonatack:
ACK abac4367607d8d2b628e4db6a9663c960bacdacc
meshcollider:
re-utACK abac4367607d8d2b628e4db6a9663c960bacdacc
Tree-SHA512: 92309fb74c48694160807326c0fe9793044a75cd77ed19400cceab54a7eefeb54ffc9334535e6021b3af7b9a364dbbeda3a9173540fff8144dfd437e96d76b5c
|
|
|
|
|
|
|
|
7bf6dfbb484adfda3b8df26ee3e2ebda239dd263 wallet: Remove path checking code from bitcoin-wallet tool (Russell Yanofsky)
77d5bb72b8722ec7a6c7c33479a532cbd5870ba4 wallet: Remove path checking code from createwallet RPC (Russell Yanofsky)
a987438e9d9cad0b5530e218a447928485f3fd93 wallet: Remove path checking code from loadwallet RPC (Russell Yanofsky)
8b5e7297c02f3100a9cb27bfe206e3fc617ec173 refactor: Pass wallet database into CWallet::Create (Russell Yanofsky)
3c815cfe54087fd139169161d2fd175e99840e6a wallet: Remove Verify and IsLoaded methods (Russell Yanofsky)
0d94e6062547f288a75921d2433458a44a5f2297 refactor: Use DatabaseStatus and DatabaseOptions types (Russell Yanofsky)
b5b414151af32e5a07b5757b64482d77519d77c0 wallet: Add MakeDatabase function (Russell Yanofsky)
288b4ffb6b291f0466d513ff3c40af6758ca7c88 Remove WalletLocation class (Russell Yanofsky)
Pull request description:
Get rid of file path handling in wallet application code and move it down to database layer.
There is no change in behavior except for some changed error messages.
Motivation for this change is to make code more understandable, but also to prepare for adding SQLite support in #19077 so SQLite implementation can be contained at the database layer and wallet loading code does not need to become more complicated.
ACKs for top commit:
achow101:
ACK 7bf6dfbb484adfda3b8df26ee3e2ebda239dd263
meshcollider:
Code re-review and functional test run ACK 7bf6dfbb484adfda3b8df26ee3e2ebda239dd263
Tree-SHA512: 23ad18324c9e8947f0cf88a3734c2e9fb25536b2cb4d552cf5d1a4ade320fbffb73bb2d1b3a99585c11630aa7092e0fcfc2dd4fe65b91e3a54161433a5cd13cb
|
|
|
|
|
|
|
|
|
|
Can be reviewed with the git diff options
--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space --ignore-all-space
|
|
fa9ee52556f493e4a896e2570ca1a3102d777d9a doc: Add doxygen comment to IsRBFOptIn (MarcoFalke)
faef4fc9b4990e563022b6ab595cb02c4060c216 Remove mempool global from interfaces (MarcoFalke)
fa831684e54783f6b40533ca218eb7636bdae667 refactor: Add IsRBFOptInEmptyMempool (MarcoFalke)
Pull request description:
The chain interface has an `m_node` member, which has a pointer to the mempool global. Use the pointer instead of the global to prepare the removal of the mempool global. See #19556
ACKs for top commit:
jnewbery:
utACK fa9ee52556
darosior:
ACK fa9ee52
hebasto:
re-ACK fa9ee52556f493e4a896e2570ca1a3102d777d9a, since my [previous](https://github.com/bitcoin/bitcoin/pull/19848#pullrequestreview-482403942) review:
Tree-SHA512: 11b4c1446f0860a743fdaa67f95c52bf0262d0a4f888be0eaf07ee497448965d32be414111bf016bd568f2989cde923430e3a3889e224057b73c499f06de7199
|
|
networks
86d4cf42d97abf4c436d1eabf29e2ed150f69c1e Increase the ip address relay branching factor for unreachable networks (Pieter Wuille)
Pull request description:
Onion addresses propagate very badly among the IPv4/IPv6 network, resulting
in difficulty for those to find each other.
The branching factor 1 is probably so low that propagations die out before
they reach another onion peer. Increase it to 1.5 on average.
ACKs for top commit:
practicalswift:
ACK 86d4cf42d97abf4c436d1eabf29e2ed150f69c1e -- patch looks correct
naumenkogs:
ACK 86d4cf4
jonatack:
ACK 86d4cf42d97abf4c436d1eabf29e2ed150f69c1e. Code review, built and running with some sanity check logging. `RelayAddress()` is called by `ProcessMessage() ADDR` msg handling, from within the loop while processing each new address to relay it to a limited number of other nodes. According to git blame, the line setting `nRelayNodes` hasn't been touched since 2016 in e736772c56a *Move network-msg-processing code out of main to its own file*, which moved the line but otherwise did not change it. Running a mixed clearnet/onion node with this patch and the logging below, I'm only seeing values of `fReachable 1, nRelayNodes 2`. IIUC, I need to use the settings in `init.cpp` that call `SetReachable(*, false)`. *Edit:* with `onlynet=onion` am now seeing entries of `fReachable 0` with `nRelayNodes` values of 1 and 2.
vasild:
ACK 86d4cf42d
Tree-SHA512: 22391e16d60bcfdec9a9336728da39d68a24a183b3d1b0e8fbc038d265ca6ddf71d16db018f3678745fd9f3e9281049e42197fa0a29124833c50a9170ed6f793
|
|
ac2ff4fb1e06270cf17727f90599c9f3a55ddd5a refactor: Avoid duplicate map lookup in ScriptToAsmStr (João Barbosa)
Pull request description:
Simple change that avoids a duplicate (unnecessary) `mapSigHashTypes` lookup.
ACKs for top commit:
laanwj:
re-ACK ac2ff4fb1e06270cf17727f90599c9f3a55ddd5a
Tree-SHA512: 7e7f5af51c1acd7a42af273e5ee5e2faddd250ba8b8f63ccb3172d95f153ae391b2816b79564b856571af52dc2a767b5736a5d10ffb5cd2c540cd9832bf86419
|
|
|
|
|
|
Co-authored-by: John Newbery <jonnynewbs@gmail.com>
|
|
|
|
CTxMemPool::GetMemPoolParents
|
|
|
