💬 1440000bytes commented on pull request "Feature: Use different datadirs for different signets":
(https://github.com/bitcoin/bitcoin/pull/29838#issuecomment-2045979791)
Concept ACK

🚀 fanquake merged a pull request: "refactor, bench, fuzz: Drop unneeded `UCharCast` calls"
(https://github.com/bitcoin/bitcoin/pull/29820)

💬 hernanmarino commented on pull request "Bugfix: bitcoin-cli: Check length of peer.transport_protocol_type":
(https://github.com/bitcoin/bitcoin/pull/29657#issuecomment-2045981680)
utACK c3e632b44153e314ef946f342c68c2758b1cbc4d

💬 sipa commented on pull request "Safegcd-based modular inverses in MuHash3072":
(https://github.com/bitcoin/bitcoin/pull/21590#issuecomment-2046018528)
Rebased, and switched to `std::countr_zero` instead of CTZ builtins.

📝 pablomartin4btc opened a pull request: "Bugfix on TransactionsView - Disable if privacy mode is set during wallet selection"
(https://github.com/bitcoin-core/gui/pull/815)
<details>
<summary>Currenlty on <code>master</code>, when the "mask values" checkbox is ticked if the user selects a different wallet, the history action is enable and if the user clicks on it can see all the transactions in the transaction view.</summary>


</details>
<details>
<summary>This PR fixes it.</summary>

![Peek 2024-04-09 17-45](https://github.com/bitcoin-core/
...

🤔 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) {
|
```