💬 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_r2189903925)
This line should have been removed indeed, as it was replaced by `ASSERT_DEBUG_LOG("Restarting logging");`.

💬 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_r2189960116)
> if the only production usage is inverted, we should likely invert the method's return value to make it more intuitive.

I disagree. Returning `true` on successful operation is intuitive to me.

💬 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_r2189920730)
Can you elaborate? Recursion is not inherently bad. It can lead to bugs, which is why we need to mindful about when we use it, but to me this looks like the best alternative. What do you suggest?

💬 enirox001 commented on issue "fuzz: Speed up mini_miner fuzz target?":
(https://github.com/bitcoin/bitcoin/issues/32870#issuecomment-3044942546)
Interested in working on speeding up the mini_miner fuzz target. Are you already on this, or should I take a stab at it?

💬 Sjors commented on pull request "wallet: allow skipping script paths":
(https://github.com/bitcoin/bitcoin/pull/32857#issuecomment-3044972740)
Sidenote: currently it's unlikely you'll accidentally send an `older()` or `after()` script path. This is because you need to manually set the input `sequence` field in the `send` (etc.) RPC (and manually adjust the fee rate). Found out the hard way while testing 🤦 (there's currently no useful feedback when this happens).

✅ maflcko closed a pull request: "New SVG, Icons, PNGs and X PixMaps"
(https://github.com/bitcoin/bitcoin/pull/32891)

💬 maflcko commented on pull request "New SVG, Icons, PNGs and X PixMaps":
(https://github.com/bitcoin/bitcoin/pull/32891#issuecomment-3045041266)
Closing for now:

* Not sure if removing shadows is the right approach to save a few KB. This needs a stronger motivation and screenshots on all supported Systems.
* Gui pull request should be opened against the gui repo: github.com/bitcoin-core/gui
* Please don't include the source code in the pull description. If anyone wanted to look at the source code, they could just look at it directly.
* Please don't ping for reviewers in the pull description. Also, pinging people that have not been
...

🤔 ryanofsky reviewed a pull request: "cmake: Move internal binaries from bin/ to libexec/"
(https://github.com/bitcoin/bitcoin/pull/31679#pullrequestreview-2993763846)
Updated 6a6b6f9093c1f82b8ea05ad87f421eabc8849738 -> f49840dd902cd9b14b6aadb431b16a4aeb719c3f ([`pr/libexec.10`](https://github.com/ryanofsky/bitcoin/commits/pr/libexec.10) -> [`pr/libexec.11`](https://github.com/ryanofsky/bitcoin/commits/pr/libexec.11), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/libexec.10..pr/libexec.11)) addressing remaining comments and making `bitcoin` help output more consistent

💬 ryanofsky commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#discussion_r2190024343)
re: https://github.com/bitcoin/bitcoin/pull/31679#discussion_r2185072353

> The table below shows the install location and availability **of** each binary after this change

Makes sense, fixed this too

💬 ryanofsky commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#discussion_r2190019152)
re: https://github.com/bitcoin/bitcoin/pull/31679#discussion_r2189175462

Oops, you're right. I forgot the `--help` help and only updated the `help` help. Sound be fixed now, thanks!

💬 ryanofsky commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#discussion_r2190022159)
re: https://github.com/bitcoin/bitcoin/pull/31679#discussion_r2189175462

> nit: we might want to realign the comments after the change

Thanks! Updated now

💬 darosior commented on pull request "BIP-119 (OP_CHECKTEMPLATEVERIFY) (regtest only)":
(https://github.com/bitcoin/bitcoin/pull/31989#issuecomment-3045062002)
I [believe](https://gnusha.org/pi/bitcoindev/F5vsDVNGXP_hmCvp4kFnptFLBCXOoRxWk9d05kSInq_kXj0ePqVAJGADkBFJxYIGkjk8Pw1gzBonTivH6WUUb4f6mwNCmJIwdXBMrjjQ0lI=@protonmail.com/) the ability to commit to the transaction to spend an output, combined with BIP340 signature verification of arbitrary messages, is a reasonable bundle of capabilities to consider for a Bitcoin soft fork. However there is clearly no consensus about this among the Bitcoin development community (see for instance [here](https://gnu
...

💬 maflcko commented on issue "fuzz: Speed up mini_miner fuzz target?":
(https://github.com/bitcoin/bitcoin/issues/32870#issuecomment-3045070337)
I am not. If you want to work on something, you can just do it without asking. In case there are two pull requests for the same thing, the authors should be glad about it, because they should be qualified to review each others pull request and pick the one that is more suitable to be merged.

💬 enirox001 commented on issue "fuzz: Speed up mini_miner fuzz target?":
(https://github.com/bitcoin/bitcoin/issues/32870#issuecomment-3045099798)
Got it.

💬 1BitcoinBoWP1FZ4xwTNkq6XksKidmgYYw commented on pull request "New SVG, Icons, PNGs and X PixMaps":
(https://github.com/bitcoin/bitcoin/pull/32891#issuecomment-3045152747)
@maflcko Thanks, I will use github.com/bitcoin-core/gui to send a new PR. Also I will squash the commits. There is a not just a few KB saved with the PR but a lot and not because removed shadows but because the files are optimized. The current files in the bitcoin/bitcoin:master are very old and not optimized.

💬 Eunovo commented on pull request "refactor: CFeeRate encapsulates FeeFrac internally":
(https://github.com/bitcoin/bitcoin/pull/32750#discussion_r2190134390)
+1 on the use of 'Assume' here.

🤔 marcofleon reviewed a pull request: "allocators: Apply manual ASan poisoning to `PoolResource`"
(https://github.com/bitcoin/bitcoin/pull/32581#pullrequestreview-2993932032)
code review ACK ad132761fc49c38769c09653a265fdbc3b93eda5

Built with ASan and ran the unit tests to be sure. Also added a small use-after-free test in `pool_tests` that passes on master but not on this PR, as expected. Nice.

💬 furszy commented on pull request "wallet: remove dead code in legacy wallet migration":
(https://github.com/bitcoin/bitcoin/pull/32758#discussion_r2190143690)
would keep the assertion. It would be bad if this is ever not the case and we end up resetting the pointer.

💬 l0rinc commented on pull request "mempool: Avoid needless vtx iteration during IBD":
(https://github.com/bitcoin/bitcoin/pull/32827#issuecomment-3045279193)
That's my expectation, yes. Would you like me to measure it to sure?

🤔 furszy reviewed a pull request: "index: fix wrong assert of current_tip == m_best_block_index"
(https://github.com/bitcoin/bitcoin/pull/32878#pullrequestreview-2994003229)
I don't think the test is useful. The change is being tested against a `Sync()` mechanism introduced within the test itself and not the real one, which can change over time (see for example #26966 that changes it completely).
It would be better, and simpler for you, to share a patch that reproduces the issue (as @mzumsande mentioned, just adding a sleep() call and triggering the reorg is enough to reproduce it).

Other than that, concept ACK.