59c0ece0a7 fuzz: replace hardcoded numbers for bech32 limits (josibake)
Pull request description:
Follow-up to #30047 to replace a hardcoded value that was missed in the original PR
ACKs for top commit:
paplorinc:
ACK 59c0ece0a7
dergoegge:
utACK 59c0ece0a7
marcofleon:
ACK 59c0ece0a7. Ran the test a bit to be sure, lgtm.
brunoerg:
utACK 59c0ece0a7
Tree-SHA512: 89799928feb6752a533259117340b087ff7299f9bf204b165dd87708e15b99a338521f2ac9f9e1fd91dc48b93be839059768d9e68b172e36328232174d1dfa3f
a0a9a11642 depends: fix ZMQ CMake getcachesize check (fanquake)
Pull request description:
Fixes#30587.
ACKs for top commit:
maflcko:
ACK a0a9a11642
hebasto:
ACK a0a9a11642. On Ubuntu (s390x), I was able to reproduce https://github.com/bitcoin/bitcoin/issues/30587. With this PR building `zeromq` succeeds.
TheCharlatan:
ACK a0a9a11642
Tree-SHA512: 70ca50ebe8e36d5a10a2354a1fbed49f5f802ae5115e09686bccca7e5d1da35168e84a6cab40dd2c83f8918889cdfdcbd5d1cbe25273b844c8ddd21865ea6c51
Erase spent cache entries and clear flags of unspent
entries inside the BatchWrite loop, instead of an
additional loop after BatchWrite.
Co-Authored-By: Pieter Wuille <pieter@wuille.net>
No visible behavior change. This commit tracks the flagged
entries internally but the list is not iterated by anything.
Co-Authored-By: Pieter Wuille <pieter@wuille.net>
Co-Authored-By: l0rinc <pap.lorinc@gmail.com>
fa895c7283 mingw: Document mode wbx workaround (MarcoFalke)
fa359255fe Add -blocksxor boolean option (MarcoFalke)
fa7f7ac040 Return XOR AutoFile from BlockManager::Open*File() (MarcoFalke)
Pull request description:
Currently the *.dat files in the blocksdir store the data received from remote peers as-is. This may be problematic when a program other than Bitcoin Core tries to interpret them by accident. For example, an anti-virus program or other program may scan them and move them into quarantine, or delete them, or corrupt them. This may cause Bitcoin Core to fail a reorg, or fail to reply to block requests (via P2P, RPC, REST, ...).
Fix this, similar to https://github.com/bitcoin/bitcoin/pull/6650, by rolling a random XOR pattern over the dat files when writing or reading them.
Obviously this can only protect against programs that accidentally and unintentionally are trying to mess with the dat files. Any program that intentionally wants to mess with the dat files can still trivially do so.
The XOR pattern is only applied when the blocksdir is freshly created, and there is an option to disable it (on creation), so that people can disable it, if needed.
ACKs for top commit:
achow101:
ACK fa895c7283
TheCharlatan:
Re-ACK fa895c7283
hodlinator:
ACK fa895c7283
Tree-SHA512: c92a6a717da83bc33a9b8671a779eeefde2c63b192362ba1d71e6535ee31d08e2802b74acc908345197de9daac6930e4771595ee25b09acd5a67f7ea34854720
f3cfbd65f5 net: log connections failures via SOCKS5 with less severity (Vasil Dimov)
Pull request description:
It is expected to have some Bitcoin nodes unreachable some of the time. A failure to connect to an IPv4 or IPv6 node is already properly logged under category=net/severity=debug. Do the same when a connection fails when using a SOCKS5 proxy. This could be either to an .onion address or to an IPv4 or IPv6 address (via a Tor exit node).
Related: https://github.com/bitcoin/bitcoin/issues/29759
ACKs for top commit:
achow101:
ACK f3cfbd65f5
mzumsande:
Code Review ACK f3cfbd65f5
tdb3:
Code Review ACK f3cfbd65f5
Tree-SHA512: c6e83568783cb5233edac7840a00f708d27be9af87480fc73093ad99fe4bd8670d3f2c97fd6b6e2c54b8d9337746eacb9a5db6eefecc1486951996bfbb0a37f7
172c1ad026 test: expand LimitOrphan and EraseForPeer coverage (Greg Sanders)
28dbe218fe refactor: move orphanage constants to header file (Greg Sanders)
Pull request description:
Inspired by refactorings in #30000 as the coverage appeared a bit sparse.
Added some minimal border value testing, timeouts, and tightened existing assertions.
ACKs for top commit:
achow101:
ACK 172c1ad026
rkrux:
reACK [172c1ad](172c1ad026)
glozow:
reACK 172c1ad026
Tree-SHA512: e8fa9b1de6a8617612bbe9b132c9c0c9b5a651ec94fd8c91042a34a8c91c5f9fa7ec4175b47e2b97d1320d452c23775be671a9970613533e68e81937539a7d70
fa530ec543 rpc: Return precise loadtxoutset error messages (MarcoFalke)
faa5c86dbf refactor: Use untranslated error message in ActivateSnapshot (MarcoFalke)
Pull request description:
The error messages should never happen in normal operation. However, if
they do, they are helpful to return to the user to debug the issue. For
example, to notice a truncated file.
This fixes https://github.com/bitcoin/bitcoin/issues/28621
Also includes a minor refactor commit.
ACKs for top commit:
fjahr:
Code review ACK fa530ec543
ryanofsky:
Code review ACK fa530ec543, just adjusting error messages a little since last review. (Thanks!)
Tree-SHA512: 224968c9b13d082ca2ed1f6a8fcc5f51ff16d6c96bd38c3679699505b54337b99cccaf7a8474391f6b11f9ccb101977b4e626898c1217eae95802e290cf105f1
2d9d752e4f scripted-diff: Replace uint256S("str") -> uint256{"str"} (Hodlinator)
c06f2368e2 refactor: Hand-replace some uint256S -> uint256 (Hodlinator)
b74d8d58fa refactor: Add consteval uint256(hex_str) (Hodlinator)
Pull request description:
Motivation:
* Validates and converts the hex string at compile time instead of at runtime into the resulting bytes.
* Makes it possible to derive other compile time constants from `uint256`.
* Potentially eliminates runtime dependencies (`SetHexDeprecated()` is called in less places).
* Has stricter requirements than the deprecated `uint256S()` (requiring 64 chars exactly, disallows garbage at the end) and replaces it in a bunch of places.
* Makes the binary smaller (tested Guix-built x86_64-linux-gnu bitcoind binary).
* Minor: should shave off a few cycles of start-up time.
Extracted from #30377 which diverged into exploring `consteval` `ParseHex()` solutions.
ACKs for top commit:
maflcko:
rebase re-cr-ACK 2d9d752e4f🎐
stickies-v:
re-ACK 2d9d752e4f
paplorinc:
ACK 2d9d752e4f
Tree-SHA512: 39bd9320db0ed81950b5d71495eaa1d06508cc008466f2308874d70ac9ff32bc69798d2e3ef6a784868c1633fb519f60cc2111a9d0718c2663b28e78b67f7cde
When using `sendrawtransaction` the ALREADY_IN_CHAIN error help string
may be confusing.
Rename TransactionError::ALREADY_IN_CHAIN to
TransactionError::ALREADY_IN_UTXO_SET and update the rpc help string.
Remove backwards compatibility alias as no longer required.
fa3ea3b83c test: Fix intermittent issue in p2p_v2_misbehaving.py (MarcoFalke)
55555574d1 net: Log accepted connection after m_nodes.push_back (MarcoFalke)
Pull request description:
Fix the two issues reported in https://github.com/bitcoin/bitcoin/pull/30468/files#r1688444784:
* Delay a debug log line for consistency.
* Fix an intermittent test issue.
They are completely separate fixes, but both `net` related.
ACKs for top commit:
0xB10C:
Code Review ACK fa3ea3b83c
stratospher:
tested ACK fa3ea3b.
Tree-SHA512: cd6b6e164b317058a305a5c3e38c56c9a814a7469039e1143f1d7addfbc91b0a28506873356b373d97448b46cb6fbe94a1309df82e34c855540b241a09489e8b
bfd3c29e4f fuzz: fix timeout in crypter target (brunoerg)
Pull request description:
Fixes#30503
- Move SetKeyFromPassphrase to out of LIMITED_WHILE
- Remove `SetKey` calls since it is already called internally by other functions.
- Reduce number of iterations (100 is enough, no need for 10,000).
ACKs for top commit:
maflcko:
review ACK bfd3c29e4f📆
dergoegge:
utACK bfd3c29e4f
Tree-SHA512: 275ab7d07a20bfd07279a23613678993c10c166f40cdc900213b9f4d5afb107462d5f88518a0f4ce2a52f3b7950ff2c01cf74292042f16996909fcb96f827d3e
chainparams.cpp - workaround for MSVC bug triggering C7595 - Calling consteval constructors in initializer lists fails, but works on GCC (13.2.0) & Clang (17.0.6).
Complements uint256::FromHex() nicely in that it naturally does all error checking at compile time and so doesn't need to return an std::optional.
Will be used in the following 2 commits to replace many calls to uint256S(). uint256S() calls taking C-string literals are littered throughout the codebase and executed at runtime to perform parsing unless a given optimizer was surprisingly efficient. While this may not be a hot spot, it's better hygiene in C++20 to store the parsed data blob directly in the binary, without any parsing at runtime.
6d33e13bd4 doc: tor.md: use -bind=127.0.0.1:8334=onion for the Tor bind (David Gumberg)
a7f5d188cc doc: add release notes for #22729 (Vasil Dimov)
Pull request description:
Add release notes for #22729.
ACKs for top commit:
davidgumberg:
reACK 6d33e13bd4
willcl-ark:
ACK 6d33e13bd4
Tree-SHA512: 9d7e66ee1d0bb1d75b8273707d30f20915d5040a768c2c5cd47c84997df2645c8bec35db6c09dc77ab917836622411b924373816cbc83c4be38e2e9156a139d8
bf0efb4fc7 scripted-diff: Modernize naming of nChainTx and nTxCount (Fabian Jahr)
72e5d1be1f test: Add basic check for nChainTx type (Fabian Jahr)
dc2938e979 chainparams: Change nChainTx to uint64_t (Fabian Jahr)
Pull request description:
This picks up the work from #29331 and closes#29258.
This simply changes the type and addresses the comments from #29331 by changing the type in all relevant places and removing unnecessary casts. This also adds an extremely simple unit test.
Additionally this modernizes the name of `nChainTx` which helps reviewers check all use of the symbol and can make silent merge conflicts.
ACKs for top commit:
maflcko:
only rebase in scripted-diff, re-ACK bf0efb4fc7🔈
glozow:
reACK bf0efb4fc7 via range-diff
Tree-SHA512: ee4020926d0800236fe655d0c7b127215ab36b553b04d5f91494f4b7fac6e1cfe7ee298b07c0983db5a3f4786932acaa54f5fd2ccd45f2fcdcfa13427358dc3b
5e87f30f7c test: check that keyless P2A 'signing' via `signrawtransactionwithkey` succeeds (Sebastian Falbesoner)
Pull request description:
This small PR adds a sanity check to verify that transactions with P2A inputs can be 'signed' successfully, using the non-wallet RPC `signrawtransactionwithkey`. Note that in the this flow, `SignStep` (which was also extended for the new `ANCHOR` output type in #30352) is never called, as signing is only tried if the locking script verification isn't successful already. See the review discussion https://github.com/bitcoin/bitcoin/pull/30352#discussion_r1690530356 ff.
ACKs for top commit:
instagibbs:
ACK 5e87f30f7c
tdb3:
ACK 5e87f30f7c
glozow:
code review ACK 5e87f30f7c
Tree-SHA512: dfea75b4bf8fa0b9c265ddd63dab36374c2430c31220f0c8eb1b53dd847c183f9e1c493a0173e2da317553a1d4cb1b35aa9ffde1268c430cc610368d23b9c942
bbcee5a0d6 clusterlin: improve rechunking in LinearizationChunking (optimization) (Pieter Wuille)
04d7a04ea4 clusterlin: add MergeLinearizations function + fuzz test + benchmark (Pieter Wuille)
4f8958d756 clusterlin: add PostLinearize + benchmarks + fuzz tests (Pieter Wuille)
0e2812d293 clusterlin: add algorithms for connectedness/connected components (Pieter Wuille)
0e52728a2d clusterlin: rename Intersect -> IntersectPrefixes (Pieter Wuille)
Pull request description:
Part of cluster mempool: #30289
Depends on #30126, and was split off from it. #28676 depends on this.
This adds the algorithms for merging & postprocessing linearizations.
The `PostLinearize(depgraph, linearization)` function performs an in-place improvement of `linearization`, using two iterations of the [Linearization post-processing](https://delvingbitcoin.org/t/linearization-post-processing-o-n-2-fancy-chunking/201/8) algorithm. The first running from back to front, the second from front to back.
The `MergeLinearizations(depgraph, linearization1, linearization2)` function computes a new linearization for the provided cluster, given two existing linearizations for that cluster, which is at least as good as both inputs. The algorithm is described at a high level in [merging incomparable linearizations](https://delvingbitcoin.org/t/merging-incomparable-linearizations/209).
For background and references, see [Introduction to cluster linearization](https://delvingbitcoin.org/t/introduction-to-cluster-linearization/1032).
ACKs for top commit:
sdaftuar:
ACK bbcee5a0d6
glozow:
code review ACK bbcee5a0d6
instagibbs:
ACK bbcee5a0d6
Tree-SHA512: d2b5a3f132d1ef22ddf9c56421ab8b397efe45b3c4c705548dda56f5b39fe4b8f57a0d2a4c65b338462d80bb5b9b84a9a39efa1b4f390420a8005ce31817774e
73e3fa10b4 doc + test: Correct uint256 hex string endianness (Hodlinator)
Pull request description:
This PR is a follow-up to #30436.
Only changes test-code and modifies/adds comments.
Byte order of hex string representation was wrongfully documented as little-endian, but are in fact closer to "big-endian" (endianness is a memory-order concept rather than a numeric concept). `[arith_]uint256` both store their data in arrays with little-endian byte order (`arith_uint256` has host byte order within each `uint32_t` element).
**uint256_tests.cpp** - Avoid using variable from the left side of the condition in the right side. Credits to @maflcko: https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1688273553
**setup_common.cpp** - Skip needless ArithToUint256-conversion. Credits to @stickies-v: https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1688621638
---
<details>
<summary>
## Logical reasoning for endianness
</summary>
1. Comparing an `arith_uint256` (`base_uint<256>`) to a `uint64_t` compares the beginning of the array, and verifies the remaining elements are zero.
```C++
template <unsigned int BITS>
bool base_uint<BITS>::EqualTo(uint64_t b) const
{
for (int i = WIDTH - 1; i >= 2; i--) {
if (pn[i])
return false;
}
if (pn[1] != (b >> 32))
return false;
if (pn[0] != (b & 0xfffffffful))
return false;
return true;
}
```
...that is consistent with little endian ordering of the array.
2. They have the same endianness (but `arith_*` has host-ordering of each `uint32_t` element):
```C++
arith_uint256 UintToArith256(const uint256 &a)
{
arith_uint256 b;
for(int x=0; x<b.WIDTH; ++x)
b.pn[x] = ReadLE32(a.begin() + x*4);
return b;
}
```
### String conversions
The reversal of order which happens when converting hex-strings <=> uint256 means strings are actually closer to big-endian, see the end of `base_blob<BITS>::SetHexDeprecated`:
```C++
unsigned char* p1 = m_data.data();
unsigned char* pend = p1 + WIDTH;
while (digits > 0 && p1 < pend) {
*p1 = ::HexDigit(trimmed[--digits]);
if (digits > 0) {
*p1 |= ((unsigned char)::HexDigit(trimmed[--digits]) << 4);
p1++;
}
}
```
Same reversal here:
```C++
template <unsigned int BITS>
std::string base_blob<BITS>::GetHex() const
{
uint8_t m_data_rev[WIDTH];
for (int i = 0; i < WIDTH; ++i) {
m_data_rev[i] = m_data[WIDTH - 1 - i];
}
return HexStr(m_data_rev);
}
```
It now makes sense to me that `SetHexDeprecated`, upon receiving a shorter hex string that requires zero-padding, would pad as if the missing hex chars where towards the end of the little-endian byte array, as they are the most significant bytes. "Big-endian" string representation is also consistent with the case where `SetHexDeprecated` receives too many hex digits and discards the leftmost ones, as a form of integer narrowing takes place.
### How I got it wrong in #30436
Previously I used the less than (`<`) comparison to prove endianness, but for `uint256` it uses `memcmp` and thereby gives priority to the *lower* bytes at the beginning of the array.
```C++
constexpr int Compare(const base_blob& other) const { return std::memcmp(m_data.data(), other.m_data.data(), WIDTH); }
```
`arith_uint256` is different in that it begins by comparing the bytes from the end, as it is using little endian representation, where the bytes toward the end are more significant.
```C++
template <unsigned int BITS>
int base_uint<BITS>::CompareTo(const base_uint<BITS>& b) const
{
for (int i = WIDTH - 1; i >= 0; i--) {
if (pn[i] < b.pn[i])
return -1;
if (pn[i] > b.pn[i])
return 1;
}
return 0;
}
```
(The commit documents that `base_blob::Compare()` is doing lexicographic ordering unlike the `arith_*`-variant which is doing numeric ordering).
</details>
ACKs for top commit:
paplorinc:
ACK 73e3fa10b4
ryanofsky:
Code review ACK 73e3fa10b4
Tree-SHA512: 121630c37ab01aa7f7097f10322ab37da3cbc0696a6bbdbf2bbd6db180dc5938c7ed91003aaa2df7cf4a4106f973f5118ba541b5e077cf3588aa641bbd528f4e
a7432dd6ed logging: clarify -debug and -debugexclude descriptions (Anthony Towns)
74dd33cb0a rpc: make logging method reject "0" category and correct the help text (Vasil Dimov)
8c6f3bf163 logging, refactor: minor encapsulation improvement and use BCLog::NONE instead of 0 (Vasil Dimov)
160706aa38 logging, refactor: make category special cases explicit (Ryan Ofsky)
Pull request description:
* Move special cases from `LOG_CATEGORIES_BY_STR` to `GetLogCategory()` (suggested [here](https://github.com/bitcoin/bitcoin/pull/29419#discussion_r1547990373)).
* Remove `"none"` and `"0"` from RPC `logging` help because that help text was wrong. `"none"` resulted in an error and `"0"` was ignored itself (contrary to what the help text suggested).
* Remove unused `LOG_CATEGORIES_BY_STR[""]` (suggested [here](https://github.com/bitcoin/bitcoin/pull/29419#discussion_r1548018694)).
This is a followup to https://github.com/bitcoin/bitcoin/pull/29419, addressing leftover suggestions + more.
ACKs for top commit:
LarryRuane:
ACK a7432dd6ed
ryanofsky:
Code review ACK a7432dd6ed. Only changes since last review are removing dead if statement and adding AJ's suggested -debug and -debugexclude help improvements, which look accurate and much more clear.
Tree-SHA512: 41b997b06fccdb4c1d31f57d4752c83caa744cb3280276a337ef4a9b7012a04eb945071db6b8fad24c6a6cf8761f2f800fe6d8f3d8836f5b39c25e4f11c85bf0
Replace early returns in KeyPair::KeyPair() with asserts.
The if statements imply there is an error we are handling, but keypair_xonly_pub
and xonly_pubkey_serialize can only fail if the keypair object is malformed, i.e.,
it was created with a bad secret key. Since we check that the keypair was created
successfully before attempting to extract the public key, using asserts more
accurately documents what we expect here and removes untested branches from the code.
Move `SignSchnorr` to `KeyPair`. This makes `CKey::SignSchnorr` now
compute a `KeyPair` object and then call `KeyPair::SignSchorr`. The
notable changes are:
* Move the merkle_root tweaking out of the sign function and into
the KeyPair constructor
* Remove the temporary secp256k1_keypair object and have the
functions access m_keypair->data() directly
Current logging RPC method documentation claims to accept "0" and "none"
categories, but the "none" argument is actually rejected and the "0"
argument is ignored. Update the implementation to refuse both
categories, and remove the help text claiming to support them.
* Make the standalone function `LogCategoryToStr()` private inside
`logging.cpp` (aka `static`) - it is only used in that file.
* Make the method `Logger::GetLogPrefix()` `private` - it is only
used within the class.
* Use `BCLog::NONE` to initialize `m_categories` instead of `0`.
We later check whether it is `BCLog::NONE` (in
`Logger::DefaultShrinkDebugFile()`).
