💬 rkrux commented on pull request "wallet: remove dead code in legacy wallet migration":
(https://github.com/bitcoin/bitcoin/pull/32758#discussion_r2189441597)
I kind of liked the separation of concerns here but done this because multiple people are interested.

💬 maflcko commented on pull request "mempool: Avoid needless vtx iteration during IBD":
(https://github.com/bitcoin/bitcoin/pull/32827#issuecomment-3044138868)
Just to clarify this is just a refactor/cleanup and doesn't affect end-to-end IBD performance in a measurable way?

💬 polespinasa commented on pull request "refactor: CFeeRate encapsulates FeeFrac internally":
(https://github.com/bitcoin/bitcoin/pull/32750#issuecomment-3044199252)
d3b8a54a81209420ef6447dd4581e1b6b8550647
Addressed some of the comments. Mostly nits.

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3044285606)
`4a873c2a67...6d697bba16`: rebase and address [the above suggestion about `BroadcastTransaction()`](https://github.com/bitcoin/bitcoin/pull/29415#pullrequestreview-2972293733).

> I think there is a bug in `BroadcastTransaction()` ...

Right, good catch! I fixed it like this:

```diff
// Transaction is not already in the mempool.
- if (max_tx_fee > 0 || broadcast_method == NO_MEMPOOL_PRIVATE_BROADCAST) {
+ const bool check_max_fee{max_tx_fee > 0};
+
...

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2189573061)
Which doc? This is in the output of `bitcoin-cli help sendrawtransaction`.

✅ maflcko closed a pull request: "ci: Catch tests corrupting the source directory"
(https://github.com/bitcoin/bitcoin/pull/32874)

💬 maflcko commented on pull request "ci: Catch tests corrupting the source directory":
(https://github.com/bitcoin/bitcoin/pull/32874#issuecomment-3044435815)
Closing for now. Probably best to move this to a dedicated nightly ci, than to try to force it into the existing ci infra.

📝 maflcko opened a pull request: "bench: Avoid tmp files in pwd"
(https://github.com/bitcoin/bitcoin/pull/32890)
It is a bit confusing that one bench run, when aborted, could leave behind temp files in the current working directory. It is similarly confusing to delete those files in the next run of bench.

Fix all issues by using `BasicTestingSetup`, which provides a proper temp folder to use and also cleans up after itself.

Can be tested via:

```
( echo 'my file content' > streams_tmp ) && ls streams_tmp && ./bld-cmake/bin/bench_bitcoin --filter=FindByte && ls streams_tmp
```

Previously the f
...

💬 maflcko commented on pull request "threading: use correct mutex name in reverse_lock fatal error messages":
(https://github.com/bitcoin/bitcoin/pull/32829#issuecomment-3044551062)
Done in https://github.com/bitcoin/bitcoin/pull/32888

💬 instagibbs commented on pull request "p2p: Relax BlockRequestAllowed to respond to advertised blocks":
(https://github.com/bitcoin/bitcoin/pull/32869#issuecomment-3044614613)
@davidgumberg might be talking past each other, but what's happening here is that locally as soon as the block is reconstructed (including merkle checks), prior to script checks, the block is advertised forward via compact blocks to its peers. This is inline with the protocol description, and helps speed up block propagation.

💬 instagibbs commented on pull request "p2p: Relax BlockRequestAllowed to respond to advertised blocks":
(https://github.com/bitcoin/bitcoin/pull/32869#discussion_r2189796353)
will do on touchup

🤔 Prabhat1308 reviewed a pull request: "ci: Use optimized Debug build type in test-each-commit"
(https://github.com/bitcoin/bitcoin/pull/32888#pullrequestreview-2993523631)
ACK [`fa21d3c`](https://github.com/bitcoin/bitcoin/pull/32888/commits/fa21d3c7781f36d96aa13fb32cad40aad5b5131f)

changes seem good as discussed in #32829 . Can we use `mold` as a linker in other Linux based system workflows ? dependencies we have seem to satisfy the deps here https://github.com/rui314/mold?tab=readme-ov-file#how-to-build

💬 maflcko commented on pull request "ci: Use optimized Debug build type in test-each-commit":
(https://github.com/bitcoin/bitcoin/pull/32888#issuecomment-3044773485)
> Can we use `mold` as a linker in other Linux based system workflows ?

Sure, happy to review a follow-up. Only place to avoid it would probably the ci tasks that mirror the guix build (win-cross, mac-cross)

💬 instagibbs commented on pull request "p2p: Relax BlockRequestAllowed to respond to advertised blocks":
(https://github.com/bitcoin/bitcoin/pull/32869#discussion_r2189897570)
fixed

💬 instagibbs commented on pull request "p2p: Relax BlockRequestAllowed to respond to advertised blocks":
(https://github.com/bitcoin/bitcoin/pull/32869#discussion_r2189897816)
done

👍 hodlinator approved a pull request: "test: fix feature_init.py intermittencies"
(https://github.com/bitcoin/bitcoin/pull/32835#pullrequestreview-2993615431)
ACK 4207d9bf823bea9f94b484ebf3c9274ca781b245

Changes since my concept review (https://github.com/bitcoin/bitcoin/pull/32835#issuecomment-3029241989):
* Addressed my feedback (https://github.com/bitcoin/bitcoin/pull/32835#pullrequestreview-2979101334)

(Would still prefer less stringly typed code, but have a feeling my suggestion in https://github.com/bitcoin/bitcoin/pull/32835#discussion_r2180861847 could probably be improved).

📝 1BitcoinBoWP1FZ4xwTNkq6XksKidmgYYw opened a pull request: "New SVG, Icons, PNGs and X PixMaps"
(https://github.com/bitcoin/bitcoin/pull/32891)
This PR clean up the old SVG, Icons, PNGs and X PixMaps by providing optimized much smaller file size versions while at the same time keeping resolutions untouched.
Shadows are removed from the files.

Ping: @jonasschnelli because he was the author of the previous files. Please check the PR.

New SVG source code:
`<?xml version="1.0" encoding="UTF-8"?>
<svg width="1024px" height="1024px" version="1.1" viewBox="0 0 1024 1024" xmlns="http://www.w3.org/2000/svg">
<g id="bitcoin">
<path
...

💬 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_r2189866340)
> Can you explain these performance reasons?

Not having to acquire a lock and iterate over an entire map.

> But shouldn't we only prefix the dropped lines by `*` anyway

No, the point is to continue making it visible to users that log lines are being suppressed, because they might have missed the initial announcement.

💬 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_r2189951621)
We're trying to get this in for 30.0. We shouldn't rush it in if we have concerns about the stability or effectiveness of this code, but blocking it for stylistic or test improvement purposes seems counter-productive when they can be done in a follow-up, ensuring progress on the important bits.

🤔 stickies-v reviewed a pull request: "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError"
(https://github.com/bitcoin/bitcoin/pull/32604#pullrequestreview-2993531256)
> My main concerns are that the code doesn't seem finished yet, it still contains commented out code, typos, redundant comments, dangerous recursion, prefixes for non-dropped log lines, duplicate state management - and I'm not even entirely sure we should be restricting logs per line in the first place.

I've responded to most of your concerns in-line. I think you've left some excellent suggestions for a follow-up, but I've not seen anything that imo warrants not merging this PR in this curren
...