👍 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).
(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
...
(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.
(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.
(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
...
(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
...
💬 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.
(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
...
(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.
(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?
(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");`.
(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.
(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?
(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?
(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).
(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)
(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
...
(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
(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
(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!
(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
(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