🤔 jarolrod reviewed a pull request: "Don't permit port in proxy IP option"
(https://github.com/bitcoin-core/gui/pull/813#pullrequestreview-1990175028)
Just to point out first, this will only work effectively in the case that editing has finished, as in clicking on another item that will take focus. If you only edit the proxy, leave focus on that box and click ok to save the setting, the invalid proxy will be saved.

Will propose a proper patch soon.

💬 0xB10C commented on pull request "tracing: Only prepare tracepoint arguments when actually tracing":
(https://github.com/bitcoin/bitcoin/pull/26593#discussion_r1558318881)
Tried this, but [didn't work for zero arguments tracepoint](https://github.com/bitcoin/bitcoin/actions/runs/8618313790/job/23620364926?pr=26593#step:6:4106) (the newly introduced tests catched this). However, we can use ` __VA_OPT__` from C++ 20.

⚠️ ryanofsky opened an issue: "RFC: "Insufficient review" tag for closed PRs"
(https://github.com/bitcoin/bitcoin/issues/29839)
During PR triage today, it seems like we closed or considered closing a number of PRs purely due to lack of review and interest in reviewing, not due to any problems with the PRs themselves.

I think it's reasonable to close PRs like these, because we have a severe lack of reviewer bandwidth, so having lots of low priority PRs open can make it harder for higher priority PRs to receive the attention they deserve.

However, I think it would be good to distinguish between PRs that are good but
...

🤔 kevkevinpal reviewed a pull request: "Logging cleanup"
(https://github.com/bitcoin/bitcoin/pull/29798#pullrequestreview-1990459252)
Concept ACK

just a question is there any reason to keep `BCLog::NONE` in `logging.h` and not completely delete it I did
`grep --exclude-dir=".deps" -nri "BCLog::NONE" ./src/ --binary-files=without-match`
and
`grep --exclude-dir=".deps" -nri "LogFlags::NONE" ./src/ --binary-files=without-match`
and only see it being used in `./src/logging.{h,cpp}` and `./src//qt/transactiondesc.cpp`

💬 kevkevinpal commented on pull request "Logging cleanup":
(https://github.com/bitcoin/bitcoin/pull/29798#discussion_r1558466877)
I noticed in 9fb321ad2b0176955718317b796715250eac01f4 that we introduce this code, I am not sure if it makes sense to remove this `0` value as the first commit so we're not adding code and removing within two commits

💬 kevkevinpal commented on pull request "test: refactor: introduce and use `calculate_input_weight` helper":
(https://github.com/bitcoin/bitcoin/pull/29777#issuecomment-2046314949)
tACK [6d91cb7](https://github.com/bitcoin/bitcoin/pull/29777/commits/6d91cb781c30966963f28e7577c7aa3829fa9390)

🤔 kevkevinpal reviewed a pull request: "i2p: fix and improve logs"
(https://github.com/bitcoin/bitcoin/pull/29833#pullrequestreview-1990642792)
lgtm ACK [64d3296](https://github.com/bitcoin/bitcoin/pull/29833/commits/64d32964892ea159d1e527756e27c416d2f66ebf)

💬 torkelrogstad commented on pull request "rpc: validate fee estimation mode case insensitive":
(https://github.com/bitcoin/bitcoin/pull/29175#issuecomment-2046591329)
Fee estimation modes are currently parsed case-insensitively here:

https://github.com/bitcoin/bitcoin/blob/e31956980e16ad3d619022e572bdf55a4eae8716/src/util/fees.cpp#L57-L67

Which makes it possible to pass arguments case-insensitively to (at least) `estimatesmartfee`:

```
$ bitcoin-cli estimatesmartfee 3 economical
{
"feerate": 0.00022015,
"blocks": 3
}

$ bitcoin-cli estimatesmartfee 3 ECONOMICAL
{
"feerate": 0.00022015,
"blocks": 3
}
```

Changing this would be
...

💬 hebasto commented on pull request "build: Enable fuzz binary in MSVC":
(https://github.com/bitcoin/bitcoin/pull/29774#issuecomment-2046626035)
Rebased on top of the merged bitcoin/bitcoin#29820 and bitcoin/bitcoin#29821.

The recent comments have been addressed.

💬 hebasto commented on pull request "build: Enable fuzz binary in MSVC":
(https://github.com/bitcoin/bitcoin/pull/29774#discussion_r1558917057)
> I'd rather we just not define this for MSVC.

Thanks! Done.

💬 instagibbs commented on pull request "policy: Allow non-standard scripts with -acceptnonstdtxn=1":
(https://github.com/bitcoin/bitcoin/pull/28334#issuecomment-2046664715)
Leaning on -0 now for this PR.

Unfortunately I think mining pools may start hacking things off with their modifications, including flipping standardness switch in mainnet. From what I can tell, Marathon was doing exactly this feedback was given to introduce back some limits, such as upgrade hooks.

💬 instagibbs commented on pull request "test: p2p: add test for rejected tx request logic (`m_recent_rejects` filter)":
(https://github.com/bitcoin/bitcoin/pull/29827#issuecomment-2046665936)
concept ack, will review once parent PR is merged

⚠️ dergoegge opened an issue: "ci: Lower and unify default stack size"
(https://github.com/bitcoin/bitcoin/issues/29840)
Default stack size differs across operating systems. E.g. the defaults for thread stack sizes: Linux: 8MiB, Windows 1MiB, FreeBSD 2MiB, OpenBSD 512 KiB.

It would be sensible to lower the stack size used in all CI jobs to the lowest default, so we don't miss stack overflow bugs that only occur with small stack sizes.

👍 TheCharlatan approved a pull request: "build: Assume HAVE_CONFIG_H, Add IWYU pragma keep to bitcoin-config.h includes"
(https://github.com/bitcoin/bitcoin/pull/29494#pullrequestreview-1990944993)
Re-ACK ffff3d2bae599d872bae184c4d7c2b0dacfc6e26

💬 instagibbs commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1558971541)
```
net_processing.cpp:3224:40: warning: comparison of unsigned expression in ‘>= 0’ is always true [-Wtype-limits]
3224 | for (auto i{package.size() - 1}; i >= 0; --i) {
|
```

💬 instagibbs commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1558979841)
this is also causing the sanitizer run failure

💬 instagibbs commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1557310808)
```suggestion
* The senders arg should be populated in same order as individual transactions
* in the package_result argument.
*/
```

💬 maflcko commented on issue "ci: Lower and unify default stack size":
(https://github.com/bitcoin/bitcoin/issues/29840#issuecomment-2046737358)
Any reason to not use half of the minimum, to more likely catch edge cases that would happen on unknown operating systems, or to more likely catch a runtime edge-case that is hit in production and not in the tests?

✅ dergoegge closed an issue: "Disallow certain message types under BIP324 v2 transport"
(https://github.com/bitcoin/bitcoin/issues/29632)

📝 fanquake opened a pull request: "fuzz: use std::source_location in fuzz/rpc.cpp"
(https://github.com/bitcoin/bitcoin/pull/29841)
Introduce a usage of [`std::source_location`](https://en.cppreference.com/w/cpp/utility/source_location).
I thought this had been/was blocked on the macOS SDK.