💬 vasild commented on pull request "Fuzz: extend CConnman tests":
(https://github.com/bitcoin/bitcoin/pull/28584#issuecomment-2922301000)
`7e42c92d2b...582d9e3dbd`: rebase due to conflicts

💬 vasild commented on pull request "Fuzz: extend CConnman tests":
(https://github.com/bitcoin/bitcoin/pull/28584#issuecomment-2922309021)
I believe all issues here have been addressed and this is ready for review. Has a stale ACK from @brunoerg and @jonatack and Concept ACK from @dergoegge.

💬 stickies-v commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2115841360)
Not a big issue, but since we have quite a few log source locations, this class will be instantiated a fair amount of (`N`) times. An alternative approach would be to have a single `LogRateLimiter` instance that encapsulates most of the logic in 6b54362c19ca9baf9dfa9be9281f6faeda169420, and has a single `m_last_reset` member, spawning a minimal `SourceLocationCounter` struct for each source location, containing just the `m_available_bytes` and `m_dropped_bytes` members.

This:
- reduces `N` `
...

💬 stickies-v commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2115859316)
Or perhaps even better than a boolean is a `uint64_t rate_limit=DEFAULT_RATE_LIMIT` so we can do away with the distinction between conditional and unconditional logging entirely? We could use rate_limit=0 as a synonym for unconditional logging, but I think even `UpdateTipLog()` doesn't have to be exempt from rate limiting, we could still set a sane value that allows doing entire IBD in one hour, for example.

Then `UpdateTipLog()` could just use:
```
LogInfo(/*rate_limit=*/100, "%s%s: ne
...

💬 stickies-v commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2115826232)
I think it's unintuitive that this function returns `true` when the operation failed. It seems like the rate limiting can be carved out pretty much as-is into a separate `bool NeedsRateLimiting()` function? There are only 2 callsites of `FormatLogStrAndRateLimit`, and the first one (`StartLogging`) doesn't look like it needs rate limiting because we already have a max buffer size?

💬 stickies-v commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2115869345)
nit: It would be nice if we could catch this in debug builds (ideally even at a fraction, say 20% of the limit), to help inform if our default limit needs to change. `Assume()` is an option, but that would probably break unit tests in debug builds.

nit, because 1MB is small enough that it should prevent issues even on devices with little disk size (unless an attacker finds multiple log vulnerabilities), and large enough that no single unconditional logging statement should ever hit that in t
...

💬 fanquake commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2115877367)
@dergoegge might remember why this was added?

💬 stickies-v commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2115895206)
fwiw: I can't find any discussion about it in https://github.com/bitcoin/bitcoin/pull/21603, and the option was present from the first version of the PR (https://github.com/bitcoin/bitcoin/commit/4648b6d207139ec0ab2994f56c0a47f81cdf516a#diff-b1e19192258d83199d8adaa5ac31f067af98f63554bfdd679bd8e8073815e69d)

💬 polespinasa commented on pull request "rpc: generateblock to allow multiple outputs":
(https://github.com/bitcoin/bitcoin/pull/32468#issuecomment-2922380999)
Rebased on top of master.

2c855694c795a31f13a473d6a6eb80c5d765e8ed implements allowing single address or descriptor without using an array or json structure as sugested in https://github.com/bitcoin/bitcoin/pull/32468#discussion_r2112898168 and https://github.com/bitcoin/bitcoin/pull/32468#discussion_r2097722076. But makes `rpc_help.py` fail. (I still don't know why)

💬 fanquake commented on pull request "depends: hard-code necessary c(xx)flags rather than setting them per-host":
(https://github.com/bitcoin/bitcoin/pull/32584#issuecomment-2922397804)
> Would be nice to explain this with commands to test and observe the difference in behavior. This would make it easier to understand the goals and test them.

Sure. On master (4b1d48a6866b24f0ed027334c6de642fc848d083):
```bash
# flags in C(XX)FLAGS
make -C depends print-linux_CFLAGS
linux_CFLAGS=-pipe -std=c11
make -C depends print-linux_CXXFLAGS
linux_CXXFLAGS=-pipe -std=c++20
```

make -C depends/ -j20 NO_QT=1 NO_WALLET=1 NO_ZMQ=1 NO_USDT=1
```bash
# base cflags (-pipe -std=c11)
...

🤔 TheCharlatan reviewed a pull request: "Add bitcoin-{node,gui} to release binaries for IPC"
(https://github.com/bitcoin/bitcoin/pull/31802#pullrequestreview-2881363308)
Concept ACK

Guix builds (aarch64)
```
eb7c84cea8ea007cba8605b256f176cfc2c4ee0229fd93a80fca0e0d5fe2f71d guix-build-af227a0e7d97/output/aarch64-linux-gnu/SHA256SUMS.part
b41de2faee47dee6947724e0bbe0dfb69fd91f7a1eb571956b03adda51c73f73 guix-build-af227a0e7d97/output/aarch64-linux-gnu/bitcoin-af227a0e7d97-aarch64-linux-gnu-debug.tar.gz
dfff9d1b7989a31dfa762195c65e09b6427d99bf97bc774fcadf6ed86be4ddbc guix-build-af227a0e7d97/output/aarch64-linux-gnu/bitcoin-af227a0e7d97-aarch64-linux-gnu.ta
...

💬 fanquake commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-2922429542)
> OpenBSD are not supported yet, the latter due to a fairly trivial upstream issue.

If it's trivial, I'd say you should pull the patch into depends, rather than opting into a different default behaviour for an entire platform?

📝 theStack opened a pull request: "fs: use `ftruncate` in `AllocateFileRange` on OpenBSD"
(https://github.com/bitcoin/bitcoin/pull/32645)
Closes issue #32643.

Tested on OpenBSD 7.7 (amd64) by running a signet IBD, with the following patch on top to ensure the fallback isn't executed anymore:
```diff
diff --git a/src/util/fs_helpers.cpp b/src/util/fs_helpers.cpp
index 84ac3690c9..c6dff2d29d 100644
--- a/src/util/fs_helpers.cpp
+++ b/src/util/fs_helpers.cpp
@@ -208,6 +208,7 @@ void AllocateFileRange(FILE* file, unsigned int offset, unsigned int length)
// OpenBSD doesn't have fallocate or posix_fallocate, use ftruncat
...

💬 fanquake commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-2922437173)
cc @theStack ^

📝 instagibbs opened a pull request: "p2p: Add mutation check inside compact block's FillBlock"
(https://github.com/bitcoin/bitcoin/pull/32646)
Since #29412, we have not allowed mutated blocks to continue being processed immediately the block is received, but this is only done for the legacy BLOCK message.

Extend these checks as belt-and-suspenders to not allow similar mutation strategies to affect relay by honest peers by applying the check inside
PartiallyDownloadedBlock::FillBlock, immediately before retruning READ_STATUS_OK.

This does result in a behavior change: a node giving a compact block that decodes successfully, but is
...

💬 instagibbs commented on pull request "p2p: Add mutation check inside compact block's FillBlock":
(https://github.com/bitcoin/bitcoin/pull/32646#issuecomment-2922447715)
cc @dergoegge @Crypt-iQ

💬 instagibbs commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2115983196)
bytes could mean serialized bytes, vbytes is less ambiguous, even if the term itself is more bespoke

💬 wilfred1005 commented on pull request "test: cover invalid codesep positions for signature in taproot":
(https://github.com/bitcoin/bitcoin/pull/32301#issuecomment-2922505039)
ISIKWEVWEN WILFRED OSARO VENTURE LTD

💬 instagibbs commented on pull request "test: cover invalid codesep positions for signature in taproot":
(https://github.com/bitcoin/bitcoin/pull/32301#issuecomment-2922510417)
rebased due to merge conflict

💬 instagibbs commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2922535795)
rebase required due to silent merge conflict