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

💬 maflcko commented on pull request "fuzz: use std::source_location in fuzz/rpc.cpp":
(https://github.com/bitcoin/bitcoin/pull/29841#issuecomment-2046766022)
-0 on this refactor. The existing code seems easier to read and is shorter?

Also, albeit not an issue here, when used as default argument the location is wrong, see https://github.com/llvm/llvm-project/issues/56379.

💬 fanquake commented on pull request "fuzz: use std::source_location in fuzz/rpc.cpp":
(https://github.com/bitcoin/bitcoin/pull/29841#issuecomment-2046772255)
> The existing code seems easier to read and is shorter?

Yea, I assumed this might be one pushback. I guess the project can decide if this is actually something we want to use in general. Or, if the tradeoff in verbosity is only worth it when used more globally.

✅ fanquake closed a pull request: "fuzz: use std::source_location in fuzz/rpc.cpp"
(https://github.com/bitcoin/bitcoin/pull/29841)

💬 fanquake commented on pull request "fuzz: use std::source_location in fuzz/rpc.cpp":
(https://github.com/bitcoin/bitcoin/pull/29841#issuecomment-2046775039)
Actually, will just come back to this after Clang 16.