💬 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_r2189897739)
> contains multiple styles

I don't think that's a good reason to hold up a useful and important PR that already has gone several iterations (in this and a previous PR) when nitting about documentation styles can block progress entirely as eventually reviewers lose interest and dilligence in re-reviewing. Follow-ups are entirely appropriate places for this to happen.

💬 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_r2189948409)
> when the logging is called via some dedicated method which hijacks the location

I don't think we can reasonably protect against malicious modifications to the binary?

> Or if macros are messing up the lines in any way.

That seems both unlikely and just something that should be, and is, tested.

> you just have to make sure you exhaust the target logger

"just" seems inappropriate, unconditional logging _should_ always take care that it can never be used to exhaust resources, but w
...

💬 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_r2189884875)
> I'm not sure I fully understand yet why we need these states

It's because the callsites behave differently whether a log line is un, newly or still suppressed. Previously, we encapsulated all that logic inside the `Consume()` function (then called `NeedsRateLimiting()`, but that gave the function too many responsibilities (as I believe you also pointed out), so this cleans that up. It think perhaps working out the code you'd prefer would be helpful.

💬 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_r2189872916)
This is a public class, explaining the parameters and interface seems very desirable to me. `scheduler_func` could mean many things, when I review code I like to be able to quickly see what something's intent is. This seems like a pretty obvious best practice?

💬 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.