-A node with bloomfilters disabled should disconnect peers that send
msg_mempool, msg_filterload, or msg_filteradd.
-Renamed the test because it now has a wider scope and msg_mempool's
actual functionality makes more sense for p2p_filter.py.
-msg_mempool is currently only tested with bloomfilter disabled
(node is disconnected) in p2p_mempool.py
-msg_mempool should get mempool txns in response when bloomfilter
is enabled
-edit test that doesn't test msg_mempool as intended
c75de5da5f [TESTS] Move base58 to own module to break circular dependency (Pieter Wuille)
Pull request description:
I encountered difficulties with the test framework in #17977. This fixes them, and I think the change is generally useful.
ACKs for top commit:
laanwj:
Code review ACK c75de5da5f
MarcoFalke:
ACK c75de5da5f according to --color-moved=dimmed-zebra this is a move-only apart from the imports ๐
Tree-SHA512: 9e0493de3e279074f0c70e92c959b73ae30479ad6f2083a3c6bbf4b0191d65ef94854559a5b7c904f5dadc5e93129ed00f6dc0a8ccce6ba7921cd45f7119f74b
d49612f98a Make SetMiscWarning() accept bilingual_str argument (Hennadii Stepanov)
d1ae7c0355 Make GetWarnings() return bilingual_str (Hennadii Stepanov)
38e33aa481 refactor: Make GetWarnings() bilingual_str aware internally (Hennadii Stepanov)
Pull request description:
This is one more step for consistent usage of `bilingual_str`.
No new translation messages are defined.
ACKs for top commit:
laanwj:
Code review ACK d49612f98a
MarcoFalke:
ACK d49612f98a๐
Tree-SHA512: 7413cb94a85291209c182845f6873350bb9e9ce940647d416c462a136603832fec8a63d792341bf634f07629767c78bc206d3a318cf10c7e87241c114c2496e9
948f1134bc gui: add missing translation.h include to fix build (fanquake)
Pull request description:
After #19176, building the gui on Bionic is failing with:
```bash
CXX qt/qt_libbitcoinqt_a-guiutil.o
qt/bitcoin.cpp: In function 'int GuiMain(int, char**)':
qt/bitcoin.cpp:460:35: error: 'Untranslated' was not declared in this scope
node->initError(strprintf(Untranslated("Error parsing command line arguments: %s\n"), error));
```
The merge commit also failed to compile with the same error:
https://travis-ci.org/github/bitcoin/bitcoin/jobs/696627543
ACKs for top commit:
hebasto:
ACK 948f1134bc, tested on Linux Mint 19.3 (x86_64): it fixes compiling error with the `--disable-wallet` configure option.
Tree-SHA512: db0197b110b3a7d05af2ceb29fbe9eeb6521d28f53b6267aa6d07a975886adb5c6485af79506ab6c66ed101e32292feeaff3707cdbc11432e5b97400953d5631
After #19176, building the gui on Bionic is failing with:
```bash
CXX qt/qt_libbitcoinqt_a-guiutil.o
qt/bitcoin.cpp: In function 'int GuiMain(int, char**)':
qt/bitcoin.cpp:460:35: error: 'Untranslated' was not declared in this scope
node->initError(strprintf(Untranslated("Error parsing command line arguments: %s\n"), error));
```
The merge commit also failed to compile with the same error:
https://travis-ci.org/github/bitcoin/bitcoin/jobs/696627543
6fe989054f refactor: Change Node::initError to take bilingual_str (Wladimir J. van der Laan)
425e7cb8cf refactor: Put`TryParsePermissionFlags` in anonymous namespace (Wladimir J. van der Laan)
77b79fa6ef refactor: Error message bilingual_str consistency (Wladimir J. van der Laan)
Pull request description:
A straightforward and hopefully uncontroversial refactor to improve consistency.
- Move the decision whether to translate an individual error message to where it is defined. This simplifies call sites: no more `InitError(Untranslated(SomeFunction(...)))`.
- Make all functions in `util/error.h` consistently return a `bilingual_str`. We've decided to use this as error message type so let's roll with it.
This has no functional changes: no messages are changed, no new translation messages are defined.
Also make a function static that can be static.
ACKs for top commit:
MarcoFalke:
ACK 6fe989054f๐ฃ
hebasto:
ACK 6fe989054f, tested on Linux Mint 19.3 (x86_64).
Tree-SHA512: 1dd123ef285c4b50bbc429b2f11c9a63aaa669a84955a0a9b8134e9dc141bc38f863f798e8982ac68bbe83170e1067a87d1a87fe7f791928b7914e10bbc2ef8d
6fc641644f change blacklist to blocklist (TrentZ)
Pull request description:
Let's use a more appropriate and clear word and discard the usage of the blacklist. Blocklist is clear and shall make everyone happy.
ACKs for top commit:
amitiuttarwar:
ACK 6fc641644f
jonatack:
ACK 6fc641644f git grep shows these two lines to be the only uses of the word in the codebase other than for specifying colors for the GUI.
sipsorcery:
ACK 6fc641644f due to easy change.
Tree-SHA512: 12fd55ad5c79f1a227da90c7fa730972aae6b74ab1f9df79ec1e7d0eca05c383ef7d6ef5f353620a01da344db915005339b62ca0884179d0f47fbefb084c9efc
2ed54da18a Merge #755: Recovery signing: add to constant time test, and eliminate non ct operators
28609507e7 Add tests for the cmov implementations
73596a85a2 Add ecdsa_sign_recoverable to the ctime tests
2876af4f8d Split ecdsa_sign logic into a new function and use it from ecdsa_sign and recovery
5e1c885efb Merge #754: Fix uninit values passed into cmov
f79a7adcf5 Add valgrind uninit check to cmovs output
05d315affe Merge #752: autoconf: Use ":" instead of "dnl" as a noop
a39c2b09de Fixed UB(arithmetics on uninit values) in cmovs
3a6fd7f636 Merge #750: Add macOS to the CI
5e8747ae2a autoconf: Use ":" instead of "dnl" as a noop
71757da5cc Explictly pass SECP256K1_BENCH_ITERS to the benchmarks in travis.sh
99bd661d71 Replace travis_wait with a loop printing "\a" to stdout every minute
bc818b160c Bump travis Ubuntu from xenial(16.04) to bionic(18.04)
0c5ff9066e Add macOS support to travis
b6807d91d8 Move travis script into a standalone sh file
f39f99be0e Merge #701: Make ec_ arithmetic more consistent and add documentation
39198a03ea Merge #732: Retry if r is zero during signing
59a8de8f64 Merge #742: Fix typo in ecmult_const_impl.h
4e284655d9 Fix typo in ecmult_const_impl.h
f862b4ca13 Merge #740: Make recovery/main_impl.h non-executable
ffef45c98a Make recovery/main_impl.h non-executable
2361b3719a Merge #735: build: fix OpenSSL EC detection on macOS
3b7d26b23c build: add SECP_TEST_INCLUDES to bench_verify CPPFLAGS
84b5fc5bc3 build: fix OpenSSL EC detection on macOS
37ed51a7ea Make ecdsa_sig_sign constant-time again after reverting 25e3cfb
93d343bfc5 Revert "ecdsa_impl: replace scalar if-checks with VERIFY_CHECKs in ecdsa_sig_sign"
7e3952ae82 Clarify documentation of tweak functions.
89853a0f2e Make tweak function documentation more consistent.
41fc785602 Make ec_privkey functions aliases for ec_seckey_negate, ec_seckey_tweak_add and ec_seckey_mul
22911ee6da Rename private key to secret key in public API (with the exception of function names)
5a73f14d6c Mention that value is unspecified for In/Out parameters if the function returns 0
f03df0e6d7 Define valid ECDSA keys in the documentation of seckey_verify
5894e1f1df Return 0 if the given seckey is invalid in privkey_negate, privkey_tweak_add and privkey_tweak_mul
8f814cddb9 Add test for boundary conditions of scalar_set_b32 with respect to overflows
3fec982608 Use scalar_set_b32_seckey in ecdsa_sign, pubkey_create and seckey_verify
9ab2cbe0eb Add scalar_set_b32_seckey which does the same as scalar_set_b32 and also returns whether it's a valid secret key
4f27e344c6 Merge #728: Suppress a harmless variable-time optimization by clang in memczero
01993878bb Add test for memczero()
52a03512c1 Suppress a harmless variable-time optimization by clang in memczero
8f78e208ad Merge #722: Context isn't freed in the ECDH benchmark
ed1b91171a Merge #700: Allow overriding default flags
85b35afa76 Add running benchmarks regularly and under valgrind in travis
ca4906b02e Pass num of iters to benchmarks as variable, and define envvar
02dd5f1bbb free the ctx at the end of bench_ecdh
e9fccd4de1 Merge #708: Constant-time behaviour test using valgrind memtest.
08fb6c4926 Run valgrind_ctime_test in travis
3d2302257f Constant-time behaviour test using valgrind memtest.
96d8ccbd16 Merge #710: Eliminate harmless non-constant time operations on secret data.
0585b8b2ee Merge #718: Clarify that a secp256k1_ecdh_hash_function must return 0 or 1
7b50483ad7 Adds a declassify operation to aid constant-time analysis.
34a67c773b Eliminate harmless non-constant time operations on secret data.
ca739cba23 Compile with optimization flag -O2 by default instead of -O3
eb45ef3384 Clarify that a secp256k1_ecdh_hash_function must return 0 or 1
856a01d6ad Merge #714: doc: document the length requirements of output parameter.
d72b9e2483 Merge #682: Remove Java Native Interface
4b48a43106 doc: document the length requirements of output parameter.
1b4d256e2e Merge #713: Docstrings
dabfea7e21 field: extend docstring of secp256k1_fe_normalize
dc7d8fd9e2 scalar: extend docstring of secp256k1_scalar_set_b32
074ab582dd Merge #704: README: add a section for test coverage
acb7f97eb8 README: add a section for test coverage
227a4f2d07 Merge #709: Remove secret-dependant non-constant time operation in ecmult_const.
d567b779fe Clarify comments about use of rzr on ge functions and abs function.
2241ae6d14 Remove secret-dependant non-constant time operation in ecmult_const.
642cd062bd Remove Java Native Interface
83fb1bcef4 Remove -O2 from default CFLAGS because this would override the -O3 flag (see AC_PROG_CC in the Autoconf manual)
ecba8138ec Append instead of Prepend user-CFLAGS to default CFLAGS allowing the user to override default variables
613c34cd86 Remove test in configure.ac because it doesn't have an effect
f45d897101 Merge #703: Overhaul README.md
2e759ec753 Overhaul README.md
d644dda5c9 Merge #689: Remove "except in benchmarks" exception for fp math
bde2a32286 Convert bench.h to fixed-point math
387d723c3f Merge #679: Add SECURITY.md
0db61d25c9 Merge #685: Fix issue where travis does not show the ./tests seedโฆ
a0771d15e6 Explicitly disable buffering for stderr in tests
fb424fbba2 Make travis show the ./tests seed by removing stdout buffering and always cat tests.log after a travis run.
22a6031184 Merge #690: Add valgrind check to travis
544002c008 Merge #678: Preventing compiler optimizations in benchmarks without a memory fence
dd98cc988f travis: Added a valgrind test without endro and enabled recovery+ecdh
b4c1382a87 Add valgrind check to travis
0c774d89e6 Merge #688: Fix ASM setting in travis
5c5f71eea5 Fix ASM setting in travis
e2625f8a98 Merge #684: Make no-float policy explicit
bae1bea3c4 Make no-float policy explicit
78c3836341 Add SECURITY.md
362bb25608 Modified bench_scalar_split so it won't get optimized out
73a30c6b58 Added accumulators and checks on benchmarks so they won't get optimized out
770b3dcd6f Merge #677: Remove note about heap allocation in secp256k1_ecmult_odd_multiples_table_storage_var
b76142ff25 Remove note about heap allocation in secp256k1_ecmult_odd_multiples_table_storage_var which was removed in 47045270fa90f81205d989f7107769bce1e71c4d
137d304a6b Merge #647: Increase robustness against UB in secp256k1_scalar_cadd_bit
0d9540b13f Merge #664: Remove mention of ec_privkey_export because it doesn't exist
59782c68b4 Remove mention of ec_privkey_export because it doesn't exist
96cd94e385 Merge #337: variable sized precomputed table for signing
dcb2e3b3ff variable signing precompute table
b4bff99028 Merge #661: Make ./configure string consistent
a467047e11 Make ./configure string consistent
e729cc7f5a Merge #657: Fix a nit in the recovery tests
b64a2e2597 Fix a nit in the recovery tests
e028aa33d3 Merge #650: secp256k1/src/tests.c: Properly handle sscanf return value
f1e11d363d Merge #654: Fix typo (โ)
ef83281c3a Merge pull request #656 from real-or-random/patch-1
556caad2ca Fix typo in docs for _context_set_illegal_callback
0d82732a9a Improve VERIFY_CHECK of overflow in secp256k1_scalar_cadd_bit. This added check ensures that any curve order overflow doesn't go undetected due a uint32_t overflow.
786dfb49f5 Merge #583: JNI: fix use sig array
e95f8ab098 Merge #644: Avoid optimizing out a verify_check
384f55606a Merge #652: README.md: update instruction to run tests
ee56accd47 Merge #651: Fix typo in secp256k1_preallocated.h
7b9b117230 Merge #640: scalar_impl.h: fix includes
d99bec2e21 Merge #655: jni: Use only Guava for hex encoding and decoding
2abcf951af jni: Use only Guava for hex encoding and decoding
271582b3b7 Fix typo
ce6d438266 README.md: update instruction to run tests
b1e68cb8e6 Fix typo in secp256k1_preallocated.h
a11c76c59a secp256k1/src/tests.c: Properly handle sscanf return value
8fe63e5654 Increase robustness against UB. Thanks to elichai2 who noted that the literal '1' is a signed integer, and that shifting a signed 32-bit integer by 31 bits causes an overflow and yields undefined behaviour. While 'scalar_low_impl''s 'secp256k1_scalar_cadd_bit' is only used for testing purposes and currently the 'bit' parameter is only 0 or 1, it is better to avoid undefined behaviour in case the used domain of 'secp256k1_scalar_cadd_bit' expands.
94ae7cbf83 Moved a dereference so the null check will be before the dereferencing
2cb73b1064 scalar_impl.h: fix includes
fa33017135 Merge #634: Add a descriptive comment for secp256k1_ecmult_const.
ee9e68cd30 Add a descriptive comment for secp256k1_ecmult_const.
d0d738d32d Merge #631: typo in comment for secp256k1_ec_pubkey_tweak_mul ()
6914c25276 typo in comment for secp256k1_ec_pubkey_tweak_mul ()
e541a90ef6 Merge #629: Avoid calling _is_zero when _set_b32 fails.
f34b0c3f35 Merge #630: Note intention of timing sidechannel freeness.
8d1563b0ff Note intention of timing sidechannel freeness.
1669bb2865 Merge #628: Fix ability to compile tests without -DVERIFY.
ecc94abcc8 Merge #627: Guard memcmp in tests against mixed size inputs.
544435fc90 Merge #578: Avoid implementation-defined and undefined behavior when dealing with sizes
143dc6e9ee Merge #595: Allow to use external default callbacks
e49f7991c2 Add missing #(un)defines to base-config.h
77defd2c3b Add secp256k1_ prefix to default callback functions
908bdce64e Include stdio.h and stdlib.h explicitly in secp256k1.c
5db782e655 Allow usage of external default callbacks
6095a863fa Replace CHECKs for no_precomp ctx by ARG_CHECKs without a return
cd473e02c3 Avoid calling secp256k1_*_is_zero when secp256k1_*_set_b32 fails.
6c36de7a33 Merge #600: scratch space: use single allocation
98836b11f0 scratch: replace frames with "checkpoint" system
7623cf2b97 scratch: save a couple bytes of unnecessarily-allocated memory
a7a164f2c6 scratch: rename `max_size` to `size`, document that extra will actually be allocated
5a4bc0bb95 scratch: unify allocations
c2b028a281 scratch space: thread `error_callback` into all scratch space functions
0be1a4ae62 scratch: add magic bytes to beginning of structure
92a48a764d scratch space: use single allocation
40839e21b9 Merge #592: Use trivial algorithm in ecmult_multi if scratch space is small
dcf392027b Fix ability to compile tests without -DVERIFY.
a484e0008b Merge #566: Enable context creation in preallocated memory
0522caac8f Explain caller's obligations for preallocated memory
238305fdbb Move _preallocated functions to separate header
695feb6fbd Export _preallocated functions
814cc78d71 Add tests for contexts in preallocated memory
ba12dd08da Check arguments of _preallocated functions
5feadde462 Support cloning a context into preallocated memory
c4fd5dab45 Switch to a single malloc call
ef020de16f Add size constants for preallocated memory
1bf7c056ba Prepare for manual memory management in preallocated memory
248bffb052 Guard memcmp in tests against mixed size inputs.
36698dcfee Merge #596: Make WINDOW_G configurable
a61a93ff50 Clean up ./configure help strings
2842dc523e Make WINDOW_G configurable
1a02d6ce51 Merge #626: Revert "Merge #620: Install headers automatically"
662918cb29 Revert "Merge #620: Install headers automatically"
14c7dbd444 Simplify control flow in DER parsing
ec8f20babd Avoid out-of-bound pointers and integer overflows in size comparisons
01ee1b3b3c Parse DER-enconded length into a size_t instead of an int
912680ed86 Merge #561: Respect LDFLAGS and #undef STATIC_PRECOMPUTATION if using basic config
91fae3ace0 Merge #620: Install headers automatically
5df77a0eda Merge #533: Make sure we're not using an uninitialized variable in secp256k1_wnaf_const(...)
975e51e0d9 Merge #617: Pass scalar by reference in secp256k1_wnaf_const()
735fbde04e Merge #619: Clear a copied secret key after negation
16e86150d0 Install headers automatically
069870d92a Clear a copied secret key after negation
8979ec0d9a Pass scalar by reference in secp256k1_wnaf_const()
84a808598b Merge #612: Allow field_10x26_arm.s to compile for ARMv7 architecture
d4d270a59c Allow field_10x26_arm.s to compile for ARMv7 architecture
248f046611 Make sure we're not using an uninitialized variable in secp256k1_wnaf_const(...)
9ab96f7b12 Use trivial algorithm in ecmult_multi if scratch space is small
dbed75d969 Undefine `STATIC_PRECOMPUTATION` if using the basic config
310111e093 Keep LDFLAGS if `--coverage`
74e2dbd68e JNI: fix use sig array
3cb057f842 Fix possible integer overflow in DER parsing
git-subtree-dir: src/secp256k1
git-subtree-split: 2ed54da18add295668ec71c91534b640d2cc029b
- Move the decision whether to translate an error message to where it is
defined. This simplifies call sites: no more `InitError(Untranslated(...))`.
- Make all functions in `util/error.h` consistently return a
`bilingual_str`. We've decided to use this as error message type so
let's roll with it.
This has no functional changes: no messages are changed, no new
translation messages are defined.
bacbfb61ee refactor: Replace RecursiveMutex with Mutex in warnings.cpp (Hennadii Stepanov)
Pull request description:
The functions that could lock this mutex, i.e., `SetMiscWarning()`, `{S,G}etfLargeWorkForkFound()`, `SetfLargeWorkInvalidChainFound()`, `GetWarnings()`, do not call itself recursively, and do not call each other either directly or indirectly. Therefore, the `g_warnings_mutex` could be a non-recursive mutex.
Related to #19180.
ACKs for top commit:
laanwj:
Code review ACK bacbfb61ee
MarcoFalke:
ACK bacbfb61ee , reviewed with -W --word-diff-regex=. ๐ฟ
Tree-SHA512: cc06d3d30e4051115d176dcfbd496c8562a70087369bccde756c1de42d7dc3f415ef20d3d69ad2599c1d0cd4228d604d7564adc17beac7b6ff92b924b8c20d54
b00266fe0c refactor: replace pointers by references within tx_verify.{h,cpp} (Sebastian Falbesoner)
Pull request description:
This PR gets rid of another unnecessary use of raw pointers, similar to PR #19053 (see also issue #19062 where useful commands for finding potential candidates are listed) but in the tx verification module.
For the functions `CalculateSequenceLocks()` and `SequenceLocks()`, the `prevHeights` vector parameter type is changed to be passed as a reference. Note that there were no checks for null pointers -- if one would pass `nullptr` to one of the functions, the following line would immediately lead to a crash:
dcacea096e/src/consensus/tx_verify.cpp (L32)
ACKs for top commit:
Empact:
Code Review ACK b00266fe0c
Tree-SHA512: 0eb71591467905434082029128bdca4df94988c372af40dca325654f6c002c72a00c73776cb5e72d6de2b2f218649211a5dbf19300a2e01f1841d6034e0f01e0
34645c4dd0 Test txinwitness is accessible on coinbase vin (Rod Vagg)
3e4421070a Expose txinwitness for coinbase in JSON form (Rod Vagg)
Pull request description:
## Rationale
The CLI can provide you with everything about transactions and blocks that you need to reconstruct the block structure and raw block itself **except** for the witness commitment nonce which is stored in the `scriptWitness` of the coinbase and is not printed. You could manually parse the raw `"hex"` fields for transactions if you really wanted to, but this seems to defeat the point of having a JSONification of the raw block/transaction data.
Without the nonce you can't:
1. calculate and validate the witness commitment yourself, you can generate the witness tx merkle root but you don't have the nonce to combine it with
2. reconstruct the raw block form because you don't have `scriptWitness` stack associated with the coinbase (although you know how big it will be and can guess the common case of `[0x000...000]`)
I'm building some archiving tooling for block data and being able to do a validated two-way conversion is very helpful.
## What
This PR simply makes the `txinwitness` field not dependent on whether we are working with the coinbase or not. So you get it for the coinbase as well as the rest.
## Examples
Common case of a `[0x000...000]` nonce: 00000000000000000000140a7289f3aada855dfd23b0bb13bb5502b0ca60cdd7
```json
"vin": [
{
"coinbase": "0368890904c1fe8d5e2f706f6f6c696e2e636f6d2ffabe6d6d5565843a681160cf7b08b1b74ac90a719e6d6ab28c16d336b924f0dc2fcabdc6010000000000000051bf2ad74af345dbe642154b2658931612a70d195e007add0100ffffffff",
"txinwitness": [
"0000000000000000000000000000000000000000000000000000000000000000"
],
"sequence": 4294967295
}
],
...
```
Novel nonce value: 000000000000000000008c31945b2012258366cc600a3e9a3ee0598e8f797731
```json
"vin": [
{
"coinbase": "031862082cfabe6d6d80c099b5e21f4c186d54eb292e17026932e52b1b807fa1380574c5adc1c843450200000000000000",
"txinwitness": [
"5b5032506f6f6c5d5b5032506f6f6c5d5b5032506f6f6c5d5b5032506f6f6c5d"
],
"sequence": 4294967295
}
],
...
```
## Alternatives
This field could be renamed for the coinbase, `"witnessnonce"` perhaps. It could also be omitted when null/zero (`0x000...000`).
## Tests
This didn't break any tests and I couldn't find an obvious way to include a test for this. If this is desired I'd apreicate some pointers.
ACKs for top commit:
MarcoFalke:
ACK 34645c4dd0
Tree-SHA512: b192facc1dfd210a5ec3f0d5d1ac6d0cae81eb35be15eaa71f60009a538dd6a79ab396f218434e7e998563f7f0df2c396cc925cb91619f6841c5a67806148c85
34e641a564 test: Remove unnecessary disconnect_nodes call in rpc_psbt.py (Danny Lee)
e6e7abd51a test: remove redundant two-way disconnect_nodes calls (Danny Lee)
a9bd1f9adf test: warn if nodes not connected before disconnect_nodes (Danny Lee)
Pull request description:
There's no harm in calling `disconnect_nodes` for nodes that weren't connected (in this case it's a no-op). However, detecting this case and logging a warning can help ensure that tests are behaving as expected.
In addition, since `disconnect_nodes` works bidirectionally, I removed all instances of this pattern:
```
disconnect_nodes(self.nodes[0], 1)
disconnect_nodes(self.nodes[1], 0)
```
ACKs for top commit:
MarcoFalke:
review ACK 34e641a564๐
amitiuttarwar:
ACK 34e641a564. Thanks for this test improvement!
Tree-SHA512: 344855ceb46c012d43c13d7c09f44d32dcb7645706d10ae1e4645d9edca54c6c6c13fee26b79480755cdfcdf39b4b5770b36bb03ce71ba002d5be8a27fe008af
This is a simple refactor of the specified test. It is now brought in
line with the rest of the tests in the module. This should make things
easier to debug, as all of the tests are now grouped together at the
top.
cb38b069b0 util: Don't reference errno when pthread fails. (MIZUTA Takeshi)
Pull request description:
Pthread library does not set errno.
Pthread library's errno is returned by return value.
ACKs for top commit:
practicalswift:
ACK cb38b069b0 -- patch looks correct
MarcoFalke:
review ACK cb38b069b0
hebasto:
ACK cb38b069b0, only squashed commits since the [previous](https://github.com/bitcoin/bitcoin/pull/19194#pullrequestreview-425831739) review.
Tree-SHA512: e6c950e30726e5031db97a7b84c8a9215da5ad3e5d233bcc349f812ad15957ddfe378e26d18339b9e0a5dcac2f50b47a687b87a6a6beaf6139df84f31531321e
fa2c2b50d8 doc: Extract net permissions doc (MarcoFalke)
Pull request description:
Moving the documentation of each flag form the already over-large init.cpp into the net permissions module should clean up the code a bit. Moreover, making the documentation available is also required for an (currently imaginary) `setnetpermissions` RPC.
ACKs for top commit:
Sjors:
re-utACK fa2c2b50d8
Tree-SHA512: c0a75facc9768913c28d2ffcdfaad8d60f7604d5584ee546adaf77d270563558d361aeaf354e49e349aca7e2e80814b27ffc24247e7b4f045c63cbdc079b449f
cc5c0d2299 refactor: Fix formatting of timedata.cpp (Hennadii Stepanov)
c2410ceb84 refactor: Replace RecursiveMutex with Mutex in timedata.cpp (Hennadii Stepanov)
Pull request description:
Only `GetTimeOffset()` and `AddTimeData()` functions lock this mutex. They do not call itself recursively, and do not call each other either directly or indirectly. Therefore, the `g_timeoffset_mutex` could be a non-recursive mutex.
Related to #19180.
ACKs for top commit:
MarcoFalke:
ACK cc5c0d2299 , checked the second commit with --word-diff-regex=. --ignore-all-space -U0 ๐ฆ
vasild:
ACK cc5c0d22 verified that recursion is not happening
Tree-SHA512: 38f6df689374d4a1a0e9aedb3ed5e885d8285c4da6b75f9bc84ae036936a159ef8276462db33b4f4dd5c71c6312fa9b45380f7a5726959665bc71dc39031be88
78c8f4fe11 refactor: Replace RecursiveMutex with Mutex in netbase.cpp (Hennadii Stepanov)
Pull request description:
The functions that could lock this mutex, i.e., `{S,G}etProxy()`, `{S,G}etNameProxy()`, `HaveNameProxy()`, `IsProxy()`, do not call itself recursively, and do not call each other either directly or indirectly. Therefore, the `g_proxyinfo_mutex` could be a non-recursive mutex.
Related to #19180.
ACKs for top commit:
MarcoFalke:
ACK 78c8f4fe11 , reviewed with the -W git option ๐ฎ
vasild:
ACK 78c8f4fe verified that recursion does not happen
Tree-SHA512: fc077fb371f38af5d05f1383c6bebf9926167c257892936fefd2d4fe6f679ca40124d25099e09f645d8ec266df222f96c5d0f9fd39eddcad15cbde0b427bc205
1a9ef1d398 refactor: Replace RecursiveMutex with Mutex in Shutdown() (Hennadii Stepanov)
Pull request description:
Step by step, going to replace all of the `RecursiveMutex` instances with the `Mutex` ones throughout the code base :)
Not sure if it is possible in all cases though...
This one is a low-hanging fruit.
ACKs for top commit:
MarcoFalke:
ACK 1a9ef1d398 Shutdown is not recursive, so the same thread can never lock twice (UB)
vasild:
ACK 1a9ef1d3 verified manually that `Shutdown()` is not called from places that could be called from inside `Shutdown()`.
Tree-SHA512: 362a507b1a6f97dc351f708224aedbfe4bee03c4398f394d78ee31c24d76a7012ffff0e6766866cd5fd9a8e0d8840f05a2741111fe583aa20d45f0af3df0dcfa
Pthread library does not set errno.
Pthread library's errno is returned by return value.
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
fac6b9b938 test: Avoid overwriting the NodeContext member of the testing setup (MarcoFalke)
fa16e7816b build: Add -Wshadow-field (MarcoFalke)
Pull request description:
Adding this warning will eliminate unexpected test failures and hard to review code. Moreover, there shouldn't be a use case in Bitcoin Core that relies on fields to be shadowed.
ACKs for top commit:
fanquake:
ACK fac6b9b938 - Warnings compiling fa16e7816b are below. No warnings with fac6b9b938. The `-Wshadow-field` diagnostic has been available in Clang since 5.0.0. It's not available for GCC.
practicalswift:
ACK fac6b9b938 -- patch looks correct
hebasto:
ACK fac6b9b938, tested on Linux Mint 19.3 (x86_64):
Tree-SHA512: 824874ca10877efea7463cf934a2953147f3f99c486f04679426c14ff968975d8652cbba5729bfb7cb2c86c637ade5d1e5b873d611c06bad013a7cad8427e2bf
This test originally made a message with an invalid stated length, and
an invalid checksum. This was because only the header was changed, but
the checksum stayed the same. This was fine for now because we check
the header first to see if it has a valid stated size, and we disconnect
if it does not, so we never end up checking for the checksum. If this
behavior was to change, this test would become a problem. (Indeed I
discovered this when playing around with this behavior). By instead
creating a message with an oversized payload from the start, we create a
message with an invalid stated length but a valid checksum, as intended.
Additionally, this takes advantage to the newly module-global
VALID_DATA_LIMIT as opposed to the magic 0x02000000. Yes, 4MB < 32MiB,
but at the moment when receiving a message we check both, so this makes
the test tighter.
As well, this renames those variables to match PEP8 and this clears up
the comment relating to VALID_DATA_LIMIT.
Admittedly, this commit is mainly to make the following ones cleaner.
