It should never be a nullopt when the transaction result is valid -
Assume() this is the case. However, as a belt-and-suspenders just in
case it is nullopt, use an empty list.
c2e0489b71 [rpc, bugfix] Enforce maximum value for setmocktime (dergoegge)
Pull request description:
The maximum value for our mocktime must be representable in nanoseconds, otherwise we end up with negative values returned from `NodeClock::now()`.
Found through fuzzing:
```
$ echo "c2V0bW9ja3RpbWVcZTptYf9w/3NldG3///////////////9p////ZP///ymL//////89////Nv9L////////LXkBAABpAA==" | base64 --decode > rpc-8cab9148ab4418ebd1923c213e9d3fe9c9b49b39.crash
$ FUZZ=rpc ./src/test/fuzz/fuzz rpc-8cab9148ab4418ebd1923c213e9d3fe9c9b49b39.crash
fuzz_libfuzzer: util/time.cpp:28: static NodeClock::time_point NodeClock::now(): Assertion `ret > 0s' failed.
```
ACKs for top commit:
maflcko:
re-ACK c2e0489b71
brunoerg:
crACK c2e0489b71
glozow:
ACK c2e0489b71
Tree-SHA512: d7e237ca37bedd74a6b085fb6e726a142705371044c77488f593f35afe70aeca756fdba86920294b1d322c7a9b2cde9ce4e1b7d410a6ccc1fd7c6f3a6e77200a
92f88a9629 doc: fixup NAT-PMP help doc (fanquake)
02395edca9 init: remove redundant upnp #ifdef (fanquake)
Pull request description:
This is a very belated followup to #26896 (which removed the configure options for setting the upnp and natpmp runtime default) and corrects the `-help` docs for `-upnp` and `-natpmp`.
ACKs for top commit:
davidgumberg:
ACK 92f88a9629
hernanmarino:
ACK 92f88a9629
Tree-SHA512: 795dc8a8703bf322b5831d845de85f2428ee0dd45d3064b48ff47d147147381af26c0a9d00c596db12009b254763844b209989daf4e7470d20e8a1753b640966
3472e2f5ec Merge sipa/minisketch#81: Avoid overflowing shift by special casing inverse of 1
653d8b2e26 Avoid overflowing shift by special casing inverse of 1
33b7c200b9 Merge sipa/minisketch#80: Add c++20 version of CountBits
4a48f31a37 Merge sipa/minisketch#83: ci: Fix "s390x (big-endian)" task
82b6488acb Add c++20 version of CountBits
0498084d31 ci: Fix "s390x (big-endian)" task
71709dca9e Merge sipa/minisketch#82: ci: Fix `x86_64-w64-mingw32` task
9e6127fa98 Merge sipa/minisketch#74: Avoid >> above type width in BitWriter
ed420bc170 ci: Fix `x86_64-w64-mingw32` task
fe1040f227 Drop -Wno-shift-count-overflow compile flag
154bcd43bd Avoid >> above type width in BitWriter
67b87acdb6 Merge sipa/minisketch#78: ci: Update macOS image for CI
7de7250416 ci: Update macOS image for CI
83d812ea9f Merge sipa/minisketch#73: ci: Use correct variable to designate C++ compiler
e051a7d690 ci: Install wine32 package for Windows tests
2d2c695d78 build: Drop unused `CC` variable
1810fcbd11 ci: Use correct variable to designate C++ compiler
022b959049 Merge sipa/minisketch#77: Add missing include
08443c4892 Add missing include
git-subtree-dir: src/minisketch
git-subtree-split: 3472e2f5ec75ace39ce9243af6b3fee233a67492
4ba1d0b553 fuzz: Add coverage for client_maxfeerate (Greg Sanders)
91d7d8f22a AcceptMultipleTransactions: Fix workspace client_maxfeerate (Greg Sanders)
f3aa5bd5eb fill_mempool: assertions and docsctring update (Greg Sanders)
a3da63e8fe Move fill_mempool to util function (Greg Sanders)
73b68bd8b4 fill_mempool: remove subtest-specific comment (Greg Sanders)
Pull request description:
Bug causes an `Assume()` failure due to the expectation that the individual result should be invalid when done over `submitpackage` via rpc.
Bug introduced by https://github.com/bitcoin/bitcoin/pull/28950 , and I discovered it rebasing https://github.com/bitcoin/bitcoin/pull/28984 since it's easier to hit in that test scenario.
Tests in place were only checking `AcceptSingleTransaction`-level checks due to package evaluation only triggering when minfee is too high for the parent transaction.
Added test along with fix, moving the fill_mempool utility into a common area for re-use.
ACKs for top commit:
glozow:
reACK 4ba1d0b553
theStack:
ACK 4ba1d0b553
ismaelsadeeq:
re-ACK 4ba1d0b553 via [diff](4fe7d150eb..4ba1d0b553)
Tree-SHA512: 3729bdf7f25d04e232f173ccee04ddbb2afdaafa3d04292a01cecf58fb11b3b2bc133e8490277f1a67622b62d17929c242dc980f9bb647896beea4332ee35306
Limit number of IPs learned from a single DNS seed to 32, to prevent the results from
one DNS seed from dominating AddrMan. Note that the number of results from a UDP DNS query is
bounded to 33 already, but it is possible for it to use TCP where a potentially enormous
number of results can be returned.
Closes #16070.
This helper class is an alternative to CMedianFilter, but without a
lot of the special logic and exceptions that we needed while it was
still used for consensus.
d5a715536e build: remove boost::process dependency for building external signer support (Sebastian Falbesoner)
70434b1c44 external_signer: replace boost::process with cpp-subprocess (Sebastian Falbesoner)
cc8b9875b1 Add `cpp-subprocess` header-only library (Hennadii Stepanov)
Pull request description:
Closes https://github.com/bitcoin/bitcoin/issues/24907.
This PR is based on **theStack**'s [work](https://github.com/bitcoin/bitcoin/issues/24907#issuecomment-1466087049).
The `subprocess.hpp` header has been sourced from the [upstream repo](https://github.com/arun11299/cpp-subprocess) with the only modification being the removal of convenience functions, which are not utilized in our codebase.
Windows-related changes will be addressed in subsequent follow-ups.
ACKs for top commit:
achow101:
reACK d5a715536e
Sjors:
re-tACK d5a715536e
theStack:
Light re-ACK d5a715536e
fanquake:
ACK d5a715536e - with the expectation that this code is going to be maintained as our own. Next PRs should:
Tree-SHA512: d7fb6fecc3f5792496204190afb7d85b3e207b858fb1a75efe483c05260843b81b27d14b299323bb667c990e87a07197059afea3796cf218ed8b614086bd3611
a71eadf66b Change MAC_OSX macro to __APPLE__ in crypto package (Lőrinc)
Pull request description:
Split out from https://github.com/bitcoin/bitcoin/pull/29450#issuecomment-2044405345 to avoid the uncertainties and simplify review.
ACKs for top commit:
theuni:
ACK a71eadf66b
fanquake:
ACK a71eadf66b
Tree-SHA512: b6a7bd7ca95585dd9110cefe7c1213f4a1a72bdfc88670abf4a0d9a8bbc12e093544524adce46aa9ca714c472d417f74ca4a678af682ed2488053059434eaa02
a8203e9412 refactor: Simplify `extra_txn` to be a vec of CTransactionRef instead of a vec of pair<Wtxid, CTransactionRef> (AngusP)
c3c18433ae refactor: Use typesafe Wtxid in compact block encoding message, instead of ambiguous uint256. (AngusP)
Pull request description:
The first commit replaces `uint256` with typesafe `Wtxid` (or `Txid`) types introduced in #28107.
The second commit then simplifies the extra tx `vector` to just be of `CTransactionRef`s instead of a `std::pair<Wtxid, CTransactionRef>`, as it's easy to get a `Wtxid` from a transaction ref.
ACKs for top commit:
glozow:
ACK a8203e9412
dergoegge:
ACK a8203e9412
Tree-SHA512: b4ba1423a8059f9fc118782bd8a668213d229c822f22b01267de74d6ea97fe4f2aad46b5da7c0178ecc9905293e9a0eafba1d75330297c055e27fd53c8c8ebfd
03b87a3e64 Drop Windows Socket dependency for `randomenv.cpp` (Hennadii Stepanov)
Pull request description:
This change drops a dependency on the ws2_32 library for our libbitcoinkernel by switching to [`GetComputerName`](https://learn.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-getcomputernamew) function.
ACKs for top commit:
sipsorcery:
utACK 03b87a3e64.
laanwj:
Code review ACK 03b87a3e64.
fanquake:
ACK 03b87a3e64
Tree-SHA512: a4abd5499176634d5f3fbf4e794a7504c40232fb73bd7f41955fbfb2cc7c44bc7ea4518c5203836e52f552c30414c6c3e1b24f0922641dbf1c8377981c0ffaf0
4f273ab436 Change Luke Dashjr seed to dashjr-list-of-p2p-nodes.us (Luke Dashjr)
Pull request description:
To avoid issues with DNS blacklisting, I've setup a separate domain for my DNS seed.
(This time, without a potentially alarming name)
ACKs for top commit:
kevkevinpal:
Concept ACK [4f273ab](4f273ab436), name looks good to me
petertodd:
ACK 4f273ab436
mzumsande:
ACK 4f273ab436
fanquake:
ACK 4f273ab436
Tree-SHA512: 689698e3c735df3ed0c2756a9d4adb5644bb9d8a6954e23d66bfa9d94ee10954f77fb241d9593f750054d731aa1532368a0fc8277884f6c2a98ac47cd0bdeeb7
2d1819455c crypto: chacha20: always use our fallback timingsafe_bcmp rather than libc's (Cory Fields)
Pull request description:
Looking at libc sources, apple and openbsd implementations match our naive fallback. Only FreeBSD (and only x86_64) seems to [implement an optimized version](https://github.com/freebsd/freebsd-src/blob/main/lib/libc/amd64/string/timingsafe_bcmp.S).
It's not worth the hassle of using a platform-specific function for such little gain.
Additionally, as mentioned below, this is the only case outside of sha2 that requires an autoconf check, and I have upcoming PRs to remove the sha2 ones.
Apple's [impl is unoptimized](https://opensource.apple.com/source/Libc/Libc-1244.1.7/string/FreeBSD/timingsafe_bcmp.c.auto.html).
As-is [OpenBSD's impl](https://github.com/openbsd/src/blob/master/lib/libc/string/timingsafe_bcmp.c).
Relevant IRC conversation with sipa:
> \<cfields\> sipa: chacha20poly1305.cpp uses libc's timingsafe_bcmp when possible. But looking around at apple/freebsd/openbsd, I don't see any impl that doesn't use the naive implementation that matches our fallback...
> \<cfields\> is there any reason to belive there's an optimized impl somewhere that we're actually hitting?
> \<cfields\> asking because after cleaning up sha2, timingsafe_bcmp is the last autoconf check that remains in all of crypto. It'd make life easy if we could just always use our internal one.
> \<cfields\> *all of crypto/
> \<sipa\> cfields: let's get rid of the dependency then
> \<sipa\> it's a trivial function
> \<sipa\> and if we need it for some platforms, no real reason not to use it on all
After the above discusstion, I did end up finding the x86_64-optimized FreeBSD impl, but I don't think that's all that significant.
ACKs for top commit:
sipa:
utACK 2d1819455c
fanquake:
ACK 2d1819455c
TheCharlatan:
ACK 2d1819455c
theStack:
ACK 2d1819455c
Tree-SHA512: b9583e19ac2f77c5d572aa5b95bc4b53669d5717e5708babef930644980de7c5d06a9c7decd5c2b559d70b8597328ecfe513375e3d8c3ef523db80012dfe9266
All `CTransactionRef` have `.GetWitnessHash()` that returns a cached `const Wtxid` (since fac1223a56),
so we don't need to pass transaction refs around with their IDs as they're easy to get from a ref.
561a650e0f test: Fix debug recommendation in argsman_tests (Fabian Jahr)
Pull request description:
There are recommendations in the `argsman_tests` comments on how to re-run and debug a test failure to see if it reflects an expected or unexpected change. The command tries to run a test in `util_tests` but this is in `argsman_tests` so the command doesn't work with just copy+paste. I didn't investigate further but I suspect that these tests were moved between files.
ACKs for top commit:
fanquake:
ACK 561a650e0f
Tree-SHA512: b3bb94ba1635c9455149b455f2b30ee37a8067a6242339531ab54d428177a288da29a4a10702652305eb34aa7638f51dad35fa6b0e7b74617e445327b8c4c053
fa9f36baba build: Remove HAVE_GMTIME_R (MarcoFalke)
fa72dcbfa5 refactor: FormatISO8601* without gmtime* (MarcoFalke)
fa2c486afc Revert "time: add runtime sanity check" (MarcoFalke)
Pull request description:
Now that the `ChronoSanityCheck` has passed for everyone with C++17 and is guaranteed by C++20 to always pass, remove it.
Also, remove `gmtime_r` and `gmtime_s` and replace them with `year_month_day`+`hh_mm_ss` from C++20.
ACKs for top commit:
sipa:
utACK fa9f36baba
fanquake:
ACK fa9f36baba - more std lib & even less stuff to port.
Tree-SHA512: a9e7e805b757b7dade0bcc3f95273a7dc4f68622630d74838339789dd203ad7542d36b2e090a93b2bc5a7ecc383207dd7ec82c68147108bdac7ce44f088c8c9a
Looking at apple/freebsd/openbsd sources, their implementations match our naive
fallback. It's not worth the hassle of using a platform-specific function for
no gain.
bbe82c116e Fix #29767, set m_synced = true after Commit() (nanlour)
Pull request description:
I think this problem https://github.com/bitcoin/bitcoin/issues/29767#issue-2216373048 is because of
in BaseIndex::Sync
61de64df67/src/index/base.cpp (L163-L168)
Setup m_synced = true; before Commit();
So this may cause a race condition window to BaseIndex::BlockConnected
61de64df67/src/index/base.cpp (L271-L274)
So i try to fix it with move m_synced = true after Commit().
Also see comment of Sync():
61de64df67/src/index/base.h (L151-L156)
I am a newcomer interested in Bitcoin, trying to become a member of the Bitcoin Core development team. Please give me some feedback if you could, as I may be doing something wrong. Thank you!
ACKs for top commit:
fjahr:
Code review ACK bbe82c116e
ryanofsky:
Code review ACK bbe82c116e
Tree-SHA512: 89a09498a232c87ef1e083d4cc4ed9bb15f045ad0624d5d150a87187b2b8a48a41137974dbc7ea5c37f73da90742c43259f5aa7f84b4179eb8d62033e44fa479
b0344c219a logging: remove unused BCLog::UTIL (Vasil Dimov)
d3b3af9034 log: deduplicate category names and improve logging.cpp (Vasil Dimov)
Pull request description:
The code in `logging.cpp` needs to:
* Get the category name given the flag (e.g. `BCLog::PRUNE` -> `"prune"`)
* Get the flag given the category name (e.g. `"prune"` -> `BCLog::PRUNE`)
* Get the list of category names sorted in alphabetical order
Achieve this by using the proper std containers. The result is
* less code (the diff of the first commit is +62 / -129)
* faster code (to linear search and no copy+sort)
* more maintainable code (the categories are no longer duplicated in `LogCategories[]` and `LogCategoryToStr()`)
This behavior is preserved:
`BCLog::NONE` -> `""` (lookup by `LogCategoryToStr()`)
`""` -> `BCLog::ALL` (lookup by `GetLogCategory("")`)
---
Also remove unused `BCLog::UTIL`.
---
These changes (modulo the `BCLog::UTIL` removal) are part of https://github.com/bitcoin/bitcoin/pull/29415 but they make sense on their own and would be good to have them, regardless of the fate of https://github.com/bitcoin/bitcoin/pull/29415. Also, if this is merged, that would reduce the size of https://github.com/bitcoin/bitcoin/pull/29415, thus the current standalone PR.
ACKs for top commit:
davidgumberg:
crACK b0344c219a
pinheadmz:
ACK b0344c219a
ryanofsky:
Code review ACK b0344c219a. Nice cleanup! Having to maintain multiple copies of the same mapping seemed messy and a like a possible footgun. I checked old and new mappings in both directions and confirmed no behavior should be changing.
Tree-SHA512: 57f87a090932f9b33dc8e075d1855dba9b71a3243a0758511745483dec2d9c46d3b532eadab297e78164c9b7caba370986ee380696a45f0778a841082f8e21a7
80f8b92f4f remove libbitcoinconsensus (fanquake)
Pull request description:
This was deprecated in `v27.0`, for removal in `v28.0`. See discussion in PR #29189.
ACKs for top commit:
theuni:
Concept ACK and light review ACK 80f8b92f4f. My only hesitation here is that (afaics?) there's now nothing keeping undesired features like threading or globals from working their way into the interpreter in future commits.
m3dwards:
Concept ACK 80f8b92f4f
TheCharlatan:
ACK 80f8b92f4f
hebasto:
ACK 80f8b92f4f, I have reviewed the code and it looks OK.
Tree-SHA512: 17a62118aeb088f2695c892bb32794dfea3061e3cb7d9e8e9f1c06c3ff6f63a7587fa532e37edbb91fbc5a19b12c9a0f8e05fa9e8864aa07f92665375d847e80
