88a79cb436b30b39d37d139da723f5a31e9d161b fix converttopsbt permitsigdata arg, add basic test (Gregory Sanders)
Pull request description:
The final check for extraneous sigdata has a flipped boolean, resulting in incorrect behavior.
Resolves https://github.com/bitcoin/bitcoin/issues/14355
Tree-SHA512: 5157a74b8ddebd7d836fba96765c4d7ed15a73d4289817353d3566a0f6803bd4bbc3f936735c517c7a83a6cbdb4052b9c61d23f6cc4ad00a6077278cd51adbd4
478c11dde326e2ff0480c14f76f9f6b52a7bdfd0 Correct scripted-diff example link (Yahia Chiheb)
Pull request description:
ACKs for top commit:
fanquake:
ACK 478c11dde326e2ff0480c14f76f9f6b52a7bdfd0
Tree-SHA512: 3bc741a79db9bd7abb17ef11f697b768565ec01303a5823ee6a7d8dfa6e888a99a15e9eda69f97a912abc3fd56a54f698f9a580596511bc9bcf62a6870b273f6
4928a995e9799c6c7ea84fa1efc4fef5b2ff7683 [doc] fix git add argument (Michael Polzer)
Pull request description:
[`A`](https://git-scm.com/docs/git-add#Documentation/git-add.txt--A) is the correct flag.
ACKs for top commit:
fanquake:
ACK 4928a995e9799c6c7ea84fa1efc4fef5b2ff7683 - checked that [`A`](https://git-scm.com/docs/git-add#Documentation/git-add.txt--A) and not `a` is the correct flag.
Tree-SHA512: 7e656ca9688b04ad2ef577aa1847799a34a377f5e6dfe4fd052a95d3dd98798dc10957e7f54164900ac1271f05e788ec4861026f53b910e369b0845532387cf4
5b59a19731827398aa32754d1f327178247d3199 Update merkle.cpp (4d55397500)
Pull request description:
Change comment from `The reason is that if the number of hashes in the list at a given time
is odd`, to ` The reason is that if the number of hashes in the list at a given level
is odd` (to be a bit more precise: replacing `time` with `level`)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improve coverage are always welcome.
* All other changes should have accompanying unit tests (see `src/test/`) or
functional tests (see `test/`). Contributors should note which tests cover
modified code. If no tests exist for a region of modified code, new tests
should accompany the change.
* Bug fixes are most welcome when they come with steps to reproduce or an
explanation of the potential issue as well as reasoning for the way the bug
was fixed.
* Features are welcome, but might be rejected due to design or scope issues.
If a feature is based on a lot of dependencies, contributors should first
consider building the system outside of Bitcoin Core, if possible.
* Refactoring changes are only accepted if they are required for a feature or
bug fix or otherwise improve developer experience significantly. For example,
most "code style" refactoring changes require a thorough explanation why they
are useful, what downsides they have and why they *significantly* improve
developer experience or avoid serious programming bugs. Note that code style
is often a subjective matter. Unless they are explicitly mentioned to be
preferred in the [developer notes](/doc/developer-notes.md), stylistic code
changes are usually rejected.
-->
<!--
Bitcoin Core has a thorough review process and even the most trivial change
needs to pass a lot of eyes and requires non-zero or even substantial time
effort to review. There is a huge lack of active reviewers on the project, so
patches often sit for a long time.
-->
ACKs for top commit:
MarcoFalke:
ACK 5b59a19731827398aa32754d1f327178247d3199
instagibbs:
ACK 5b59a19731
Tree-SHA512: 30d29b9855b30de8b54033ca4884cfb5bf8ab9e52cf61da237abba0e15ebff947c65f8ba82175694bc60ee0d54f940a098cadcb0404d3c3bcf577006ab0561a5
2a6bce482c13cff37c1af00231265de4656a454b doc: Add a note about backporting (Carnhof Daki)
Pull request description:
See laanwj's comment in #17158https://github.com/bitcoin/bitcoin/pull/17158#issuecomment-542627090
Top commit has no ACKs.
Tree-SHA512: ac5248a796050ce1a5bd0718955f941f6a3c025e192599948f12566eb55296079404b999676b9a2c8fe10616fc8334698dfa415af0fb4db6c98038d52218af1f
582e66b6e75d58033987a7b0474226cfdd724ce0 doc: Added regtest config for linearize script (Gr0kchain)
Pull request description:
Updated the example-linearize.cfg file to include support for the regtest chain network config which is used by the ./linearize-data.py
Problem:
Without the regtest magic, genesis hash and path config, the `linearize-data.py` script cannot generate a bootstrap.dat file.
Example:
./linearize-data.py ./linearize.cfg
Read 102 hashes
Genesis block not found in hashlist
Solution:
Added netmagic, genesis and input example parameters to file.
Resolution
1. Starting bitcoind in regtest mode
2. bitcoin-cli generatetoaddress 101 $(bitcoin-cli getnewaddress)
3. ./linearize-hashes.py ./linearize.cfg > ./hashlist.txt
4. ./linearize-data.py ./linearize.cfg
```
$ ./linearize-data.py ./linearize.cfg
Read 102 hashes
Input file /Users/gr0kchain/.bitcoin/regtest/blocks/blk00000.dat
Output file /Users/gr0kchain/Downloads/bootstrap.dat
Done (102 blocks written)
```
ACKs for top commit:
fanquake:
ACK 582e66b6e75d58033987a7b0474226cfdd724ce0
Tree-SHA512: 699e92e740e68e2e5190ba37538efbbe3e4d4e725ebd6af704a0cf5517683b691754f7ea097bf840845d2b53b793c63258d406e9bd37922db810cf58bed053c3
6c223152238d2e818e38357b03f38a4dbe9de016 build: add additional attributes to Win installer (fanquake)
Pull request description:
Fixes: #17170.
ACKs for top commit:
hebasto:
ACK 6c223152238d2e818e38357b03f38a4dbe9de016, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: d2ff2006b8df6a34b3a16270d3eb895b03cf6b3ca69404bc39adeb7d5e3b896ddab6ba831566dc966d8bdfba3f57ddf325762cddf3ad76d1427971d1bcc68255
317fb96de9c6257972f1213b4ef2c3fe87dde99f Add search for first blk file with pruned node (Rjected)
Pull request description:
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improve coverage are always welcome.
* All other changes should have accompanying unit tests (see `src/test/`) or
functional tests (see `test/`). Contributors should note which tests cover
modified code. If no tests exist for a region of modified code, new tests
should accompany the change.
* Bug fixes are most welcome when they come with steps to reproduce or an
explanation of the potential issue as well as reasoning for the way the bug
was fixed.
* Features are welcome, but might be rejected due to design or scope issues.
If a feature is based on a lot of dependencies, contributors should first
consider building the system outside of Bitcoin Core, if possible.
* Refactoring changes are only accepted if they are required for a feature or
bug fix or otherwise improve developer experience significantly. For example,
most "code style" refactoring changes require a thorough explanation why they
are useful, what downsides they have and why they *significantly* improve
developer experience or avoid serious programming bugs. Note that code style
is often a subjective matter. Unless they are explicitly mentioned to be
preferred in the [developer notes](/doc/developer-notes.md), stylistic code
changes are usually rejected.
-->
When bitcoind is running in pruned mode, producing a hashlist with `./linearize-hashes.py linearize.cfg > hashlist.txt` and then executing `linearize-data.py linearize.cfg` will produce:
```
Read 313001 hashes
Input file /home/dan/.bitcoin/blocks/blk00000.dat
Premature end of block data
```
This happens because `linearize-data` starts by attempting to process `blk00000.dat` regardless of whether or not `blk00000.dat` actually exists - this may not be the case if working with a pruned node.
This PR adds a function which finds the first block file that does exist, and calls that function when the `BlockDataCopier` is initialized.
This is a refactor of #16431.
<!--
Bitcoin Core has a thorough review process and even the most trivial change
needs to pass a lot of eyes and requires non-zero or even substantial time
effort to review. There is a huge lack of active reviewers on the project, so
patches often sit for a long time.
-->
ACKs for top commit:
darosior:
ACK 317fb96de9c6257972f1213b4ef2c3fe87dde99f
laanwj:
Code review ACK 317fb96de9c6257972f1213b4ef2c3fe87dde99f
theStack:
Code review ACK 317fb96de9
Tree-SHA512: fc8014282df6cfe7b267e64db8ce7d82b86b758c302fbfea4a3c39b62d93512f5c2e31a0de4e9c5ec18fc0268c917f011257d37b45afaef6033eec90e4aa585f
0661a3c4a6ac7e9aa24812dcd1c3ca9053248aff build: Add default configure cache to .gitignore (Hennadii Stepanov)
Pull request description:
Ref: [Autoconf - 7.4.2 Cache Files](https://www.gnu.org/savannah-checkouts/gnu/autoconf/manual/autoconf-2.69/autoconf.html#Cache-Files)
ACKs for top commit:
fanquake:
ACK 0661a3c4a6ac7e9aa24812dcd1c3ca9053248aff - sure; going to merge this. However lets not start adding every file that might occur from using any autoconf option to our .gitignore..
Tree-SHA512: 8be8fdd7fda35ae190c1613e5b3ac4860d6f9ec08f06b66b1278be26e11a1616ec781e0b88d0761690c99600b4de2306c01dd9798f9143531ddacb373e3fc677
6094222de7820d235e6e8c66e589aa71db08c077 use preferred shebang approach for documentation (hackerrdave)
Pull request description:
Documentation update to use recommended shebang approach mentioned in the [developer notes](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#shebang)
ACKs for top commit:
hebasto:
ACK 6094222de7820d235e6e8c66e589aa71db08c077, I have reviewed the code, and it looks OK, I agree it can be merged.
Tree-SHA512: fc58632f0a6fa82c7abdddfac4897f082110d647426d2b468cba6fabf6b34a015fcad47e5b26be98e629b8b0417b8781e8d89da67189e20da228b97b17f1a532
4f4ae6f97e210fa0a2aa274bcd2a77a226fe6a7e build: set AC_PREREQ to 2.69 (fanquake)
Pull request description:
We use build macros such as `AX_CHECK_LINK_FLAG`, that require >=2.64, so our configure should also require Autoconf >= 2.64. The build would already blow up if 2.64 wasn't available. i.e:
```bash
configure.ac:320: error: Autoconf version 2.64 or higher is required
build-aux/m4/ax_check_link_flag.m4:74: AX_CHECK_LINK_FLAG is expanded from...
```
For reference, Autoconf 2.69 was released in [April of 2012](https://lists.gnu.org/archive/html/autoconf/2012-04/msg00041.html).
See the [Autoconf Versioning docs](https://www.gnu.org/software/autoconf/manual/autoconf-2.69/html_node/Versioning.html) for more info on `AC_PREREQ`.
ACKs for top commit:
hebasto:
re-ACK 4f4ae6f97e210fa0a2aa274bcd2a77a226fe6a7e, Autoconf 2.69 seems wide available.
laanwj:
ACK 4f4ae6f97e210fa0a2aa274bcd2a77a226fe6a7e
Tree-SHA512: b77de9164ae6667513d40edaf9e16c6e7734c100643297b2dbb2ff54072774fdeab7b3b15d52979b99e204c1c4dcca4725ff155d7f6fdab7a867629130e10185
faede70882b4fd54390f5205dbe1dbcf019195c8 doc: Add formatting to the good first issue template (MarcoFalke)
Pull request description:
Add minor formatting to the good first issue template so that it is easier to see with one glance what the required skills are.
Preview is here: https://github.com/MarcoFalke/bitcoin-core/issues/new/choose
ACKs for top commit:
fanquake:
ACK faede70882b4fd54390f5205dbe1dbcf019195c8
Tree-SHA512: 0b0fcd051166981455061442e69f42c9fa726eaa228856e57434e012f7224781f4f3f12c31ce0a7a322df9999e79a8fbe63bf800b7933bc52c7cdaed90f37598
1c26c16065182ca2d2cdbb05fae79cac8c75f17d Improve "Hide" button tool-tip message (Danny-Scott)
Pull request description:
Cleaned up the tool tip text, it looks as though it just got included back in 2014 when the whole section was added.
Changed hide button tool tip within transaction fee settings area from "collapse fee-settings" to "Hide transaction fee settings" to be more user friendly and fit with other tool tips.
ACKs for top commit:
laanwj:
ACK 1c26c16065182ca2d2cdbb05fae79cac8c75f17d
Tree-SHA512: e2c83271c273f785ac625da9f88e095076043e21a9c59792049c271747837d19483e0cae5466c26ef3231947b6245680c4c136a530ba6f1885f9ddc18f2560d6
fcb72616253ed22e364bc312992d77efc1c4a3c1 Prevent valgrind false positive in rest_blockhash_by_height (Russell Yanofsky)
Pull request description:
A bad interaction between valgrind and clang 6.0.0-1ubuntu2 with -O2 optimizations makes valgrind misleadingly imply C++ code is reading an uninitialized blockheight value in `rest_blockhash_by_height` just because that's what clang optimized code is doing. The C++ code looks like:
```c++
int32_t blockheight;
if (!ParseInt32(height_str, &blockheight) || blockheight < 0) {
```
while the optimized code looks like:
```
0x00000000000f97ab <+123>: callq 0x4f8860 <ParseInt32(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, int*)>
0x00000000000f97b0 <+128>: mov 0xc(%rsp),%ebx
0x00000000000f97b4 <+132>: test %ebx,%ebx
0x00000000000f97b6 <+134>: js 0xf98aa <rest_blockhash_by_height(util::Ref const&, HTTPRequest*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)+378>
0x00000000000f97bc <+140>: xor $0x1,%al
0x00000000000f97be <+142>: jne 0xf98aa <rest_blockhash_by_height(util::Ref const&, HTTPRequest*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)+378>
```
During the rest_interface.py test:
eef90c14ed/test/functional/interface_rest.py (L266)
when `height_str` is empty, `ParseInt32` returns false and `blockheight` value is never assigned. The optimized code reads the uninitialized `blockheight` value in `0xc(%rsp)` before the checking the `ParseInt32` return value in `%al`, which is harmless, but triggers the following error from valgrind:
```
==30660== Thread 13 b-httpworker.2:
==30660== Conditional jump or move depends on uninitialised value(s)
==30660== at 0x2017B6: rest_blockhash_by_height(util::Ref const&, HTTPRequest*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (rest.cpp:614)
==30660== by 0x2041B9: operator() (rest.cpp:670)
==30660== by 0x2041B9: std::_Function_handler<bool (HTTPRequest*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&), StartREST(util::Ref const&)::$_1>::_M_invoke(std::_Any_data const&, HTTPRequest*&&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (std_function.h:301)
==30660== by 0x3EC994: operator() (std_function.h:706)
==30660== by 0x3EC994: HTTPWorkItem::operator()() (httpserver.cpp:55)
==30660== by 0x3ED16D: WorkQueue<HTTPClosure>::Run() (httpserver.cpp:114)
==30660== by 0x3E9168: HTTPWorkQueueRun(WorkQueue<HTTPClosure>*, int) (httpserver.cpp:342)
==30660== by 0x3EDAAA: __invoke_impl<void, void (*)(WorkQueue<HTTPClosure> *, int), WorkQueue<HTTPClosure> *, int> (invoke.h:60)
==30660== by 0x3EDAAA: __invoke<void (*)(WorkQueue<HTTPClosure> *, int), WorkQueue<HTTPClosure> *, int> (invoke.h:95)
==30660== by 0x3EDAAA: _M_invoke<0, 1, 2> (thread:234)
==30660== by 0x3EDAAA: operator() (thread:243)
==30660== by 0x3EDAAA: std:🧵:_State_impl<std:🧵:_Invoker<std::tuple<void (*)(WorkQueue<HTTPClosure>*, int), WorkQueue<HTTPClosure>*, int> > >::_M_run() (thread:186)
==30660== by 0x64256DE: ??? (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.25)
==30660== by 0x54876DA: start_thread (pthread_create.c:463)
==30660== by 0x6DC888E: clone (clone.S:95)
==30660== Uninitialised value was created by a stack allocation
==30660== at 0x20173A: rest_blockhash_by_height(util::Ref const&, HTTPRequest*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (rest.cpp:608)
==30660==
{
<insert_a_suppression_name_here>
Memcheck:Cond
fun:_ZL24rest_blockhash_by_heightRKN4util3RefEP11HTTPRequestRKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEE
fun:operator()
fun:_ZNSt17_Function_handlerIFbP11HTTPRequestRKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEEEZ9StartRESTRKN4util3RefEE3$_1E9_M_invokeERKSt9_Any_dataOS1_S9_
fun:operator()
fun:_ZN12HTTPWorkItemclEv
fun:_ZN9WorkQueueI11HTTPClosureE3RunEv
fun:_ZL16HTTPWorkQueueRunP9WorkQueueI11HTTPClosureEi
fun:__invoke_impl<void, void (*)(WorkQueue<HTTPClosure> *, int), WorkQueue<HTTPClosure> *, int>
fun:__invoke<void (*)(WorkQueue<HTTPClosure> *, int), WorkQueue<HTTPClosure> *, int>
fun:_M_invoke<0, 1, 2>
fun:operator()
fun:_ZNSt6thread11_State_implINS_8_InvokerISt5tupleIJPFvP9WorkQueueI11HTTPClosureEiES6_iEEEEE6_M_runEv
obj:/usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.25
fun:start_thread
fun:clone
}
```
This is a known bad interaction between clang and valgrind. The clang optimized code is correct but valgrind has no way of knowing that accessing the uninitialized value isn't a problem. Issue has been reported previously:
- https://bugs.llvm.org/show_bug.cgi?id=32604#c4
- https://github.com/Z3Prover/z3/issues/972
This commit just sets blockheight to -1 as a workaround.
This change was originally made in 41d5d651594c6c939add7a58b7e30c97dccdf24a from #18740 to fix the travis error there (https://travis-ci.org/github/bitcoin/bitcoin/jobs/678453061#L7157) but MarcoFalke suggested https://github.com/bitcoin/bitcoin/pull/18740#discussion_r414772851 moving to a new PR, since apparently the error's been seen on travis previously
ACKs for top commit:
MarcoFalke:
ACK fcb72616253ed22e364bc312992d77efc1c4a3c1
practicalswift:
ACK fcb72616253ed22e364bc312992d77efc1c4a3c1
Tree-SHA512: ec8abf45bd3d6c6e0e7e404d0b2a749efd43910619b84b0b5fe7dab22881598d1011a0f3ff2e146bf46320b63eb152bf63c62c06f1ab84c35dd640abc468f18f
da0842dcd44f8c9c9b167917fac0949b4978c3b0 build: Update ax_boost_mase.m4 to the latest serial (Hennadii Stepanov)
Pull request description:
Picked from the upstream 90814f1895Fix#17010.
This PR is [alternative](https://github.com/bitcoin/bitcoin/issues/17010#issuecomment-610651736) to #18501.
ACKs for top commit:
laanwj:
ACK da0842dcd44f8c9c9b167917fac0949b4978c3b0
Tree-SHA512: 5e43e12c524e4ea6b967c9be02c81a75948eac6cf55b819e3339222a2e3414731581d40af3524ad865abae7c5247c190448ebf2aa5e0d9a338edb501cc23ba38
f65c9ad40f2f5cdc581bdaf72e7dc68e9d7f7a80 Check for overflow when calculating sum of outputs (Elichai Turkel)
Pull request description:
This was reported by practicalswift here #18046
The exact order of the if, is important, we first do `!MoneyRange(tx_out.nValue)` to make sure the amount is non-negative. and then `std::numeric_limits<CAmount>::max() - tx_out.nValue < nValueOut` checks that the addition cannot overflow (if we won't check that the amount is positive this check can also overflow! (by doing something like `max - -max`))
and only then we make sure that the some is also valid `!MoneyRange(nValueOut + tx_out.nValue)`
if any of these conditions fail we throw.
the overflowing logic:
```
a + b > max // we want to fail if a+b is more than the maximum -> will overflow
b > max - a
max - a < b
```
Closes: #18046
ACKs for top commit:
MarcoFalke:
ACK f65c9ad40f2f5cdc581bdaf72e7dc68e9d7f7a80, checked that clang with O2 produces identical binaries 💕
practicalswift:
ACK f65c9ad40f2f5cdc581bdaf72e7dc68e9d7f7a80
instagibbs:
utACK f65c9ad40f
vasild:
ACK f65c9ad40f2f5cdc581bdaf72e7dc68e9d7f7a80 modulo `s/assert.h/cassert/`
Tree-SHA512: 512d6cf4762f24c41cf9a38da486b17b19c634fa3f4efbdebfe6608779e96fc3014d5d2d29adb8001e113152c0217bbd5b3900ac4edc7b8abe77f82f36209e33
e980214bc4fd49530e8d50fe0a6657b8583bc6b5 serialization: prevent int overflow for big Coin::nHeight (pierrenn)
Pull request description:
This is an attempt to fix fuzzer issues 1,2,8 reported by practicalswift here : https://github.com/bitcoin/bitcoin/issues/18046
The fuzzer harness doesn't prevent deserialization of unrealistic high values for `Coin::nHeight`. In the [provided examples](https://github.com/bitcoin/bitcoin/issues/18046), we have :
- `blockundo_deserialize` : the varint `0x8DD88DD700` is deserialized as `3944983552` in `Coin::nHeight` (`TxInOutFormatter::Unser`)
- `coins_deserialize` : the varint `0x8DD5D5EC40` is deserialized as `3939874496` similarly
- `txundo_deserialize`: the varint `0x8DCD828F01` is deserialized as `3921725441` in `Coin::nHeight` (`Coin::Unserialize`)
Since `Coin::nHeight` is 31 bit long, multiplying a large value by 2 triggers the fuzzer.
AFAIK those values are unrealistic (~70k years for the smallest..). I've looked a bit a reducing the range of values the fuzzer can deserialize, but this seems to be too much code change for not much.
Hence this PR chooses to static cast `nHeight` when re-serializing; it seems to be the less intrusive/safest way to prevent the fuzzer output.
Another more "upstream" approach would be to limit `Coin::nHeight` values to something more realistic, e.g. `0xFFFFFFF` (~5k years) :
de3a30bab2/src/undo.h (L39) and de3a30bab2/src/coins.h (L71)
Thanks !
NB: i was also not sure about the component/area to prefix the PR/commit with.. ?
ACKs for top commit:
practicalswift:
ACK e980214bc4fd49530e8d50fe0a6657b8583bc6b5 -- patch looks correct
promag:
ACK e980214bc4fd49530e8d50fe0a6657b8583bc6b5.
sipa:
utACK e980214bc4fd49530e8d50fe0a6657b8583bc6b5
MarcoFalke:
re-ACK e980214bc4fd49530e8d50fe0a6657b8583bc6b5 🎑
ryanofsky:
Code review ACK e980214bc4fd49530e8d50fe0a6657b8583bc6b5. Just removed ternary ? 1 : 0 and replaced / 2 with >> 1 since last review
Tree-SHA512: 905fc9e5e52a6857abee4a1c863751767835965804bb8c39474f27a120f65399ff4ba7a49ef1da0ba565379f8c12095bd384b6c492cf06776f01b2db68d522b8
d831831822885717e9841f1ff67c19add566fa45 lockedpool: When possible, use madvise to avoid including sensitive information in core dumps (Luke Dashjr)
Pull request description:
If we're mlocking something, it's because it's sensitive information. Therefore, don't include it in core dump files, ~~and unmap it from forked processes~~.
The return value is not checked because the madvise calls might fail on older kernels as a rule (unsure).
ACKs for top commit:
practicalswift:
Code review ACK d831831822885717e9841f1ff67c19add566fa45 -- patch looks correct
laanwj:
ACK d831831822885717e9841f1ff67c19add566fa45
jonatack:
ACK d831831822885717e9841f1ff67c19add566fa45
vasild:
ACK d831831822885717e9841f1ff67c19add566fa45
Tree-SHA512: 9a6c1fef126a4bbee0698bfed5a01233460fbcc86380d984e80dfbdfbed3744fef74527a8e3439ea226167992cff9d3ffa8f2d4dbd5ae96ebe0c12f3eee0eb9e
7a6627ae87b637bf32c03122865402bd71adf0d1 Fix mining to an invalid target + ensure that a new block has the correct hash internally in Python tests (Samer Afach)
Pull request description:
Test with block 47 in the `feature_block.py` creates a block with a hash higher than the target, which is supposed to fail. Now two issues exist there, and both have low probability of showing up:
1. The creation is done with `while (hash < target)`, which is wrong, because hash = target is a valid mined value based on the code in the function `CheckProofOfWork()` that validates the mining target:
```
if (UintToArith256(hash) > bnTarget)
return false;
```
2. As we know the hash stored in CBlock class in Python is stateful, unlike how it's in C++, where calling `CBlock::GetHash()` will actively calculate the hash and not cache it anywhere. With this, blocks that come out of the method `next_block` can have incorrect hash value when `solve=False`. This is because the `next_block` is mostly used with `solve=True`, and solving does call the function `rehash()` which calculates the hash of the block, but with `solve=False`, nothing calls that method. And since the work to be done in regtests is very low, the probably of this problem showing up is very low, but it practically happens (well, with much higher probability compared to issue No. 1 above).
This PR fixes both these issues.
Top commit has no ACKs.
Tree-SHA512: f3b54d18f5073d6f1c26eab89bfec78620dda4ac1e4dde4f1d69543f1b85a7989d64c907e091db63f3f062408f5ed1e111018b842819ba1a5f8348c7b01ade96
686c5456f2fcf7e301907223d16a85f7eb378c6c Fix missing header in sync.h (João Barbosa)
Pull request description:
`std::string` is referenced in `sync.h` but the relevant header is not explicitly included as required by current guideline. Furthermore on osx 10.14.6 with clang-900.0.31 the following error occurs:
```
In file included from threadinterrupt.cpp:6:
In file included from ./threadinterrupt.h:8:
./sync.h:206:21: error: implicit instantiation of undefined template 'std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >'
std::string lockname;
```
ACKs for top commit:
practicalswift:
ACK 686c5456f2fcf7e301907223d16a85f7eb378c6c
laanwj:
ACK 686c5456f2fcf7e301907223d16a85f7eb378c6c
Tree-SHA512: 7c1acdfa5b0dd148d1114e14c9450d5907006e63e1a04e82ed8a1e29757925476e6f8ef6024b0c6d1bb596623115209ad580d5035be1e4785337bd01b738c9f2
ddc7e42d600a0cb3e763cda0dc04a1f2f34e9440 build: Set minimum Automake version to 1.13 (Hennadii Stepanov)
Pull request description:
This PR suggests to set the required minimum Automake version to `1.13` explicitly for the following reasons:
- it guarantees that [CVE-2012-3386](https://lists.gnu.org/archive/html/automake/2012-07/msg00023.html) has been fixed
- `AC_CONFIG_MACRO_DIR` macro support, which we already use; from the [release notes](https://lists.gnu.org/archive/html/automake/2012-12/msg00038.html):
> Improvements to aclocal and related rebuilds rules:
> - Autoconf-provided macros AC_CONFIG_MACRO_DIR and AC_CONFIG_MACRO_DIRS are now traced by aclocal, and can be used to declare the local m4 include directories. Formerly, one had to specify it with an explicit '-I' option to the 'aclocal' invocation.
- `AM_SILENT_RULES` macro support (since version `1.11`)
Automake `1.13` requires Autoconf `2.65` or greater. We already have `2.69` since #17769.
---
For reference, Automake `1.13` was released in [December of 2012](https://lists.gnu.org/archive/html/automake/2012-12/msg00038.html).
CentOS 7 uses Automake [`1.13.4`](https://centos.pkgs.org/7/centos-x86_64/automake-1.13.4-3.el7.noarch.rpm.html)
See the Automake docs for more info:
- [`AM_INIT_AUTOMAKE`](https://www.gnu.org/software/automake/manual/automake.html#Public-Macros)
- [List of Automake options](https://www.gnu.org/software/automake/manual/automake.html#List-of-Automake-options)
ACKs for top commit:
laanwj:
so also ACK ddc7e42d600a0cb3e763cda0dc04a1f2f34e9440
fanquake:
ACK ddc7e42d600a0cb3e763cda0dc04a1f2f34e9440 - I think adding a minimum required version here is fine. I'd be surprised if someone who is currently building Bitcoin Core was unable to after this change.
Tree-SHA512: a1f97864bc3a513450c03d041498f28e823e6f8cd9710d81df081435d72bd4b6cd2f3deb997dbf902f950215a859e48a2ee7ca1f8ebf4271778dd951ab78abf4
fab7d14ea5a4305317d66f35beb3225a07823d42 test: Check that wait_until returns if time point is in the past (MarcoFalke)
Pull request description:
Add an explicit regression test for the condvar bug (#18227), so that this doesn't happen again
ACKs for top commit:
laanwj:
ACK fab7d14ea5a4305317d66f35beb3225a07823d42
Tree-SHA512: 6ec0d0b3945cae87a001e367af34cca1953a8082b4a0d9f8a20d30acd1f36363e98035d4eb173ff786cf6692d352d41f960633415c46394af042eb44e3b5ad71
aff2748f8aee72f03b5fb6f6f97f0d0f66391755 httpserver: use own HTTP status codes (Filip Gospodinov)
Pull request description:
Before, macros defined in `<event2/http.h>` have been used for some HTTP status codes. `<event2/http.h>` is included implicitly and the usage of its status code macros is inconsistent with the majority HTTP response implementations in this file.
Now, the `HTTPStatusCode` enum from `<rpc/protocol.h>` is consistently used for all HTTP response implementations.
ACKs for top commit:
practicalswift:
ACK aff2748f8aee72f03b5fb6f6f97f0d0f66391755 -- patch looks correct
laanwj:
ACK aff2748f8aee72f03b5fb6f6f97f0d0f66391755
Tree-SHA512: 6a7043488b88dcd584215d16b5f16f7bd668fe5553d31298d1beef134d2b0648aef81533014e34d1cd600baa36ee4e853f195352f4a00c866bd5ab1ff688bd50
fac52dafa013047b051ca7163cc30ac69ad35531 test: Set catch_system_errors=no on boost unit tests (MarcoFalke)
Pull request description:
Closes#16700
Can be tested by adding an `assert(0)` and then running either `make check` or `./src/test/test_bitcoin -t bla_tests --catch_system_errors=no/yes`
ACKs for top commit:
practicalswift:
ACK fac52dafa013047b051ca7163cc30ac69ad35531
Empact:
Tested ACK fac52dafa0
Tree-SHA512: ec00636951b2c1137aaf43610739d78d16f823f7da76a726d47f93b8b089766fb66b21504b3c5413bcf8b6b5c3db0ad74027d677db24a44487d6d79a6bdee2e0
d6d2602a32251c1017da88b47c801b7283c66ce3 add: test that transactions expire from mempool (0xb10c)
Pull request description:
This adds the functional test `mempool_expiry.py` covering mempool transaction expiry. Both the default `DEFAULT_MEMPOOL_EXPIRY` of 336 hours (two weeks, set in #9312) and the user definable mempool expiry via the `-mempoolexpiry=<n>` command line option are tested. The test checks that descendants of expired transactions are removed as well.
*Notes for reviewers*
- `LimitMempoolSize()` (which is the only caller of `CTxMemPool::Expire()`) is only called when a transaction is added to the mempool. In order to test expiry of a transaction-that-should-expire, the mocktime is set and a random transaction is broadcast to trigger `LimitMempoolSize()`. The transaction-that-should-expire is then checked for expiry. LMK if there is another way, but I don't think there is.
ACKs for top commit:
MarcoFalke:
ACK d6d2602a32251c1017da88b47c801b7283c66ce3
theStack:
ACK d6d2602a32
promag:
Code review ACK d6d2602a32251c1017da88b47c801b7283c66ce3.
Tree-SHA512: eb68cd9e2d870872b8e8e1522fed8954fb99cc9e4edda4b28bb2a4e41cddbc53fe6f7d9c090f1e0e98ab49beb24bf37ff3787a9e9801a95e8ae9ca9eb34fe6f0
2f5f7d6b135e4eab368bbafd9e6e979aa72398de GuessVerificationProgress: cap the ratio to 1 (darosior)
Pull request description:
Noticed `getblockchaininfo` would return a `verificationprogress` > 1, especially while generating. This caps the verification progress to `1`.
Tried to append a check to functional tests but this would pass even without the patch, so it seems better to not add a superfluous check (but this can easily be reproduced by trying to generate blocks in the background and `watch`ing `getblockchainfo`).
ACKs for top commit:
laanwj:
ACK 2f5f7d6b135e4eab368bbafd9e6e979aa72398de
promag:
ACK 2f5f7d6b135e4eab368bbafd9e6e979aa72398de.
Tree-SHA512: fa3aca12acab9c14dab3b2cc94351082f548ea6e6c588987cd86e928a00feb023e8112433658a0e85084e294bfd940eaafa33fb46c4add94146a0901bc1c4f80
2a95c7c95690112a03b14ccb0fb8f66db12cb75b ci: Check for submodules (Emil Engler)
Pull request description:
See #18019.
The current solution looks like this (I also tested with multiple submodules):
```
These submodules were found, delete them:
355a5a310019659d9bf6818d2fd66fbb214dfed7 curl (curl-7_68_0-108-g355a5a310)
```
The submodule example command was `git submodule add https://github.com/curl/curl.git curl`
ACKs for top commit:
laanwj:
ACK 2a95c7c95690112a03b14ccb0fb8f66db12cb75b
Tree-SHA512: 64bf388123f0a88d12e3e41ff29bc190339377a0615c35dc3f2700bb7773470a8fa426e0ff57188a60ed88bded39f75082ff0b73118651ff403b163422395005
530d02addbfea01ab24a2acd17af456a1e7b798a build: pass -fno-ident in Windows gitian descriptor (fanquake)
Pull request description:
`-fno-ident` prevents compilers from emitting compiler name and version number information that can needlessly bloat binaries.
For example, in the `v0.19.0.1` Windows release binaries, there are > 1000 GCC compiler version strings embedded:
```bash
# GCC: (GNU) 7.3-posix 20180312... & GCC: (GNU) 6.3.0 20170415.......
strings bitcoind.exe | rg GCC | wc -l
1021
```
They end up collected in the end of the`.rdata` section, and cannot be removed by `strip`. i.e:
```bash
objdump --section=.rdata --full-contents bitcoind.exe
...
cfcc00 00000000 00000000 00000000 00000000 ................
cfcc10 00000000 00000000 00000000 00000000 ................
cfcc20 4743433a 2028474e 55292036 2e332e30 GCC: (GNU) 6.3.0
cfcc30 20323031 37303431 35000000 00000000 20170415.......
cfcc40 4743433a 2028474e 55292037 2e332d70 GCC: (GNU) 7.3-p
cfcc50 6f736978 20323031 38303331 32000000 osix 20180312...
cfcc60 4743433a 2028474e 55292037 2e332d70 GCC: (GNU) 7.3-p
cfcc70 6f736978 20323031 38303331 32000000 osix 20180312...
```
The flag is available for [Clang](https://clang.llvm.org/docs/ClangCommandLineReference.html#cmdoption-clang-qn) and [GCC](https://gcc.gnu.org/onlinedocs/gcc/Code-Gen-Options.html#index-fno-ident).
Relevant code in [GCC](https://github.com/gcc-mirror/gcc/blob/master/gcc/toplev.c#L565-L578):
```c
/* Attach a special .ident directive to the end of the file to identify
the version of GCC which compiled this code. The format of the .ident
string is patterned after the ones produced by native SVR4 compilers. */
if (!flag_no_ident)
{
const char *pkg_version = "(GNU) ";
char *ident_str;
if (strcmp ("(GCC) ", pkgversion_string))
pkg_version = pkgversion_string;
ident_str = ACONCAT (("GCC: ", pkg_version, version_string, NULL));
targetm.asm_out.output_ident (ident_str);
}
```
ACKs for top commit:
practicalswift:
ACK 530d02addbfea01ab24a2acd17af456a1e7b798a
laanwj:
ACK 530d02addbfea01ab24a2acd17af456a1e7b798a
Tree-SHA512: b3b28f43ec483dee28d1df8548fe72425bf00e750701825c256395f6aa7b23256eb27609b51779b86aed108b6eaa3912181a9d8282e23eebf9cee7784f9fabe0
22c5a986e95d2bd14273465ca0e15fbe3772252d depends: Consistent use of package variable (Peter Bushnell)
Pull request description:
All other mk files use the package variable consistently except for the two instances here, which have always been here, since depends was introduced in 0.10.
ACKs for top commit:
fanquake:
ACK 22c5a986e95d2bd14273465ca0e15fbe3772252d - tested a `make boost -C depends/ -j8`.
Tree-SHA512: 41766a328603db2ebb1f23ea0c5b2936de043587dd86396eaba73524d2f5bdeff25447040e33d61de2ef612a920281cd81c6fac097913270287f344beb839c5d
d97fac422eaaefe13e1ed376e617882a100872ae Add a link from ZMQ doc to ZMQ example in contrib/ (Damian Mee)
Pull request description:
No code changes :). Only a small convenience improvement in zmq doc.
ACKs for top commit:
fanquake:
ACK d97fac422eaaefe13e1ed376e617882a100872ae
Tree-SHA512: f05a8a7a77c0a698637fd24ffc94d0d617743b434f46695a56576a53331ede254aeece416baf3f8275ae4dfad85ae6e14d1920aa32af53150847420a176d90fb
fa09110ebb5e485b17a767fca198819fcbe7c16e doc: Fix typo in Coin doxygen comment (MarcoFalke)
Pull request description:
`CTxOutCompressor` has been renamed in commit 4de934b9b5b4be1bac8fe205f4ee9a79e772dc34, so rename it in the docs as well.
ACKs for top commit:
laanwj:
ACK fa09110ebb5e485b17a767fca198819fcbe7c16e
hebasto:
ACK fa09110ebb5e485b17a767fca198819fcbe7c16e
Tree-SHA512: e16a21ac3112a67ee7d5ffabb3f47103aed8f91fdebf1bf96311cd0b7bdb9b7323ed826bfa95517386d4128ff0ae2c7c13bad047a7c5a0cc2458be7a43119157
ff6549c3c84ca7324032dbc37744645bf2fe1c3e fix: update rest info on block size and json (Chris Abrams)
Pull request description:
Addressing the ambiguous block size text in rest docs: https://github.com/bitcoin/bitcoin/issues/18703
Also makes sure to let developers know there is `.json` option for the rest output format.
ACKs for top commit:
MarcoFalke:
ACK ff6549c3c84ca7324032dbc37744645bf2fe1c3e
promag:
ACK ff6549c3c84ca7324032dbc37744645bf2fe1c3e.
Tree-SHA512: 9ef93c1432d650b1f9599778ba092c1ca5b084a537af257078e1c713c76c5d3a4cc4b1ede8a2489964be8ed0303ad8bea58c1cb4759bbb9b24dbdebfec8001d3
bd44711e1bb2eee7646f2f8e2e8763d1c216bdb9 build: pass -dead_strip_dylibs to ld on macOS (fanquake)
Pull request description:
This strips some unused dylibs from bitcoin-qt.
```diff
otool -L src/qt/bitcoin-qt
/usr/lib/libSystem.B.dylib
- /System/Library/Frameworks/DiskArbitration.framework/Versions/A/DiskArbitration
/System/Library/Frameworks/IOKit.framework/Versions/A/IOKit
/System/Library/Frameworks/Foundation.framework/Versions/C/Foundation
/System/Library/Frameworks/CoreServices.framework/Versions/A/CoreServices
/System/Library/Frameworks/AppKit.framework/Versions/C/AppKit
/System/Library/Frameworks/ApplicationServices.framework/Versions/A/ApplicationServices
/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation
-/System/Library/Frameworks/Security.framework/Versions/A/Security
/System/Library/Frameworks/SystemConfiguration.framework/Versions/A/SystemConfiguration
/System/Library/Frameworks/CoreGraphics.framework/Versions/A/CoreGraphics
/System/Library/Frameworks/OpenGL.framework/Versions/A/OpenGL
-/System/Library/Frameworks/AGL.framework/Versions/A/AGL
/System/Library/Frameworks/Carbon.framework/Versions/A/Carbon
/usr/lib/libc++.1.dylib
/System/Library/Frameworks/CFNetwork.framework/Versions/A/CFNetwork
/System/Library/Frameworks/CoreText.framework/Versions/A/CoreText
/System/Library/Frameworks/ImageIO.framework/Versions/A/ImageIO
/usr/lib/libobjc.A.dylib
```
`AGL` - ObjC wrapper for OpenGL.
`DiskArbitration` - mount/unmount notifications and events.
`Security` - low level security operations, authentication services.
From `man ld`:
```
Remove dylibs that are unreachable by the entry point or exported symbols.
That is, suppresses the generation of load command commands for dylibs
which supplied no symbols during the link. This option should not be
used when linking against a dylib which is required at runtime for
some indirect reason such as the dylib has an important initializer.
```
ACKs for top commit:
theuni:
ACK bd44711e1bb2eee7646f2f8e2e8763d1c216bdb9.
Tree-SHA512: 9592ce2966d28cb6c58e01efd401f56a4baa5dc5be5313f4fe8454632b578608be65a23c8602772049cd4655a9cb020fdd40d6622a244c301920d8c3db43f99a
651c636f9ed4a60c4cd003e566e3ac6ae6eda3ed build: Fix configure report about qr (Hennadii Stepanov)
Pull request description:
On master (b7bc9b8330096d1f4f1fa563b855b88da425226e):
```
$ apt list libqrencode-dev
Listing... Done
libqrencode-dev/bionic 3.4.4-1build1 amd64
$ ./configure | grep -i qr
checking for QR... no
checking whether to build GUI with support for QR codes... no
with qr = auto
```
With this PR:
```
$ apt list libqrencode-dev
Listing... Done
libqrencode-dev/bionic 3.4.4-1build1 amd64
$ ./configure | grep -i qr
checking for QR... no
checking whether to build GUI with support for QR codes... no
with qr = no
```
ACKs for top commit:
laanwj:
Concept and light code review ACK 651c636f9ed4a60c4cd003e566e3ac6ae6eda3ed
fanquake:
ACK 651c636f9ed4a60c4cd003e566e3ac6ae6eda3ed
Tree-SHA512: 8959b1c7da5b28d06affcdd27ff4e455f1f7d9c8363dbde8ef07aaf79139ec8bc7ce25610b28e1d90c7e168573ee90ac9ab359bf10c667d0254507f8a880a935
adbe15504713ddba6e9c024c59d977675d49e350 doc: Add some better examples for scripted diff (Wladimir J. van der Laan)
Pull request description:
The current example isn't too great, for example it uses `find` instead of `git ls-files`. Add a subsection with suggestions and examples.
Feel free to propose some other great examples to add.
ACKs for top commit:
hebasto:
re-ACK adbe15504713ddba6e9c024c59d977675d49e350
Tree-SHA512: 38f03716a122a1791c93abc052ea7572a3d2108b3d0d93dc95d3c4a7eb190c6b639d1cc66e4f74d378c4b11d6951dbd901d0973792f8f13cbeb9d9dcf4f8e037
7b78b8d3a64503d582af8298241a20ebf582a0c5 doc: Add template for good first issues (Michael Folkson)
Pull request description:
closes#17317
Attempted to address everyone's suggestions in #17317 without making it too long. The first half is for the benefit of the individual opening the issue and the second half is for the benefit of the new contributor. Ideally we don't want the second half to be deleted by the individual opening the issue but whether they delete the first half or not isn't really a concern
ACKs for top commit:
MarcoFalke:
ACK 7b78b8d3a64503d582af8298241a20ebf582a0c5
jonatack:
ACK 7b78b8d3a64503d582af8298241a20ebf582a0c5
Tree-SHA512: 5874b244a52f432637600a73aac493972971568f8d8af10aa731b8a6b221566015827dd82c310c60a76fb01140c3bc56a691206c3442018611c820d4b98d104f
a35b6824f3a0bdb68c5aef599c0f17562689970e Add assertion to randrange that input is not 0 (Jeremy Rubin)
Pull request description:
From the comment in randrange, their is an implicit argument that randrange cannot accept an argument of 0. If the argument is 0, then we have to return {}, which is not possible in a uint64_t.
The current code takes a very interesting approach, which is to return [0..std::numeric_limits<uint64_t>]. This can cause all sorts of fun problems, like allocating a lot of memory, accessing random memory (maybe with your private keys), and crashing the computer entirely.
This gives us three choices of how to make it "safe":
1) return Optional<uint64_t>
2) Change the return type to [0..range]
3) Return 0 if 0
4) Assert(range)
So which solution is best?
1) seems a bit overkill, as it makes any code using randrange worse.
2) Changing the return type as in 2 could be acceptable, but it imposes the potential overflow checking on the caller (which is what we want).
3) An interesting option -- effective makes the return type in {0} U [0..range]. But this is a bad choice, because it leads to code like `vec[randrange(vec.size())]`, which is incorrect for an empty vector. Null set should mean null set.
4) Assert(range) stands out as the best mitigation for now, with perhaps a future change to solution 2. It prevents the error from propagating at the earliest possible time, so the program crashes cleanly rather than by freezing the computer or accessing random memory.
ACKs for top commit:
instagibbs:
Seems reasonable for now, ACK a35b6824f3
laanwj:
ACK a35b6824f3a0bdb68c5aef599c0f17562689970e
promag:
ACK a35b6824f3a0bdb68c5aef599c0f17562689970e.
Tree-SHA512: 8fc626cde4b04b918100cb7af28753f25ec697bd077ce0e0c640be0357626322aeea233e3c8fd964ba1564b0fda830b7f5188310ebbb119c113513a4b89952dc
5710dadf9b282524fddf42c682351cd8022ed7bf test: fix script_p2sh_tests OP_PUSHBACK2/4 missing (kodslav)
Pull request description:
Cleans up #15140 which fixes commit 6b25f29a91 where opcodes were lost in translation.
ACKs for top commit:
laanwj:
code review ACK 5710dadf9b282524fddf42c682351cd8022ed7bf
Tree-SHA512: 3f7fbcaf0dd199626d9ec9fdf3c5b5c5c2a91c4cfe81fae5b1d5662a48e52cf4bd27c94f8f42ebdfe7a076c5d600ada5661a6902b03eb5dc3dc953f4524345ac
60582d6060542c1e3a23141ea825e36818fbbd54 [linter] Strip trailing / in path for git-subtree-check (John Newbery)
Pull request description:
git-subtree-check fails if the directory is given with a trailing slash,
eg:
```
> test/lint/git-subtree-check.sh src/univalue/
ERROR: src/univalue/ is not a subtree
```
Shell autocompletes will add the trailing slash when autofilling the
path name, which will therefore cause the script to fail.
Just ignore any trailing slash.
ACKs for top commit:
laanwj:
ACK 60582d6060542c1e3a23141ea825e36818fbbd54
dongcarl:
ACK 60582d6060542c1e3a23141ea825e36818fbbd54
fanquake:
ACK 60582d6060542c1e3a23141ea825e36818fbbd54 - tested before and after.
Tree-SHA512: 5a91979b60e1d4b1310fd02a0ccc5465dbff57d9c94bba81e4758442a627cfa32217ab8f973990a17b5d961ecae61fb56b56ccf10f87e61dd03e88a1e0b8f99d
