💬 MarcoFalke commented on pull request "refactor: Extract common/args from util/system":
(https://github.com/bitcoin/bitcoin/pull/27419#discussion_r1157536088)
Can you explain this change?

💬 MarcoFalke commented on pull request "ci: use clang-16 in tidy task":
(https://github.com/bitcoin/bitcoin/pull/27404#issuecomment-1496322016)
> This error can be fixed with the following diff:

I wonder if another workaround would be to use libc++, but I haven't checked if the iwyu output is better or worse with libc++.

💬 brunoerg commented on pull request "logging, net: add ASN from peers on logs":
(https://github.com/bitcoin/bitcoin/pull/27412#issuecomment-1496323887)
> It looks like logasmap is still added in the last push, though.

Sorry it was a leftover, fixed it.

💬 MarcoFalke commented on pull request "test: Remove windows workaround in authproxy":
(https://github.com/bitcoin/bitcoin/pull/27418#issuecomment-1496324726)
re-ran the windows test 12 times with only one failure: https://cirrus-ci.com/task/6093556574584832 . Which looks like an unrelated issue.

📝 theuni opened a pull request: "build: remove ancient unused define"
(https://github.com/bitcoin/bitcoin/pull/27420)
The generic define was removed in [upstream miniupnpc in 2014](https://github.com/miniupnp/miniupnp/commit/f6774e33169b3101c3a242984510c9b6da033e26).

Noticed while reviewing @hebasto's new CMake buildsystem: https://github.com/hebasto/bitcoin/pull/12#discussion_r1156267350.

👍 hebasto approved a pull request: "build: remove ancient unused define"
(https://github.com/bitcoin/bitcoin/pull/27420)
ACK 9fbc5fcd28eeefd3ad1932a2d94e5558deeb16d7

💬 jonatack commented on pull request "Add "warnings", deprecate "warning" in {create,load,unload,restore}wallet":
(https://github.com/bitcoin/bitcoin/pull/27279#issuecomment-1496350013)
Thanks for testing!

> I did a quick search for other RPCs that string together warnings, I think `getblockchaininfo` still does - worth including here?

Info getter RPCs `getblockchaininfo`, `getmininginfo` and `getnetworkinfo` return a different kind of warnings -- global ones unrelated to an operation performed by the RPC -- and have their own `GetWarnings` helper in `src/warnings`, which is also called by the GUI via `getWarnings`. They don't seem tightly related to the ones here.

> A
...

💬 Sjors commented on pull request "logging, net: add ASN from peers on logs":
(https://github.com/bitcoin/bitcoin/pull/27412#issuecomment-1496354235)
tACK 7b302e1

The second commit message could be improved: the current implementation _only_ logs outbound.

Would be nice to log this for inbound connections too, I guess the `receive version message:` is most suitable for that?

💬 Sjors commented on issue "ASN-based bucketing of the network nodes":
(https://github.com/bitcoin/bitcoin/issues/16599#issuecomment-1496367967)
During Advancing Bitcoin 2023 @virtu showed some simulations for the number of inbound connections nodes on "obscure" ASN's would get. Perhaps there's a way to reduce the bandwidth burden for these nodes.

It's good in general to connect to a diverse set of ASN's so we don't get eclipsed. But for downloading blocks it seems unnecessary, since we know exactly what we're looking for. Perhaps during IBD (and long catchup) we could make additional outbound connections to a fast ASN. Hardcoding Ama
...

💬 furszy commented on pull request "wallet: coin selection, don't return results that exceed the max allowed weight":
(https://github.com/bitcoin/bitcoin/pull/26720#discussion_r1157609917)
hmm why?
`warning: 'const' type qualifier on return type has no effect`

💬 furszy commented on pull request "wallet: coin selection, don't return results that exceed the max allowed weight":
(https://github.com/bitcoin/bitcoin/pull/26720#discussion_r1157613803)
pushed

💬 furszy commented on pull request "wallet: coin selection, don't return results that exceed the max allowed weight":
(https://github.com/bitcoin/bitcoin/pull/26720#discussion_r1157613941)
pushed

💬 kouloumos commented on issue "ASN-based bucketing of the network nodes":
(https://github.com/bitcoin/bitcoin/issues/16599#issuecomment-1496408484)
> During Advancing Bitcoin 2023 @virtu showed some simulations for the number of inbound connections nodes on "obscure" ASN's would get.

For more context: https://github.com/virtu/talks/tree/master/2023-03-02-advancing-bitcoin

💬 martinus commented on pull request "tracepoints: Disables `-Wgnu-zero-variadic-macro-arguments` to compile without warnings":
(https://github.com/bitcoin/bitcoin/pull/27401#issuecomment-1496410442)
> Maybe add a check to configure.ac whether ignoring of -Wgnu-zero-variadic-macro-arguments is https://github.com/bitcoin/bitcoin/issues/26916#issuecomment-1400156218 required?

Sounds good, but I have no idea how to do this with `configure.ac`... But it's also possible to do this in code level, I've now added a `__cplusplus < 202002L` so disabling the warning only happens when compiling below c++20

💬 furszy commented on pull request "wallet: coin selection, don't return results that exceed the max allowed weight":
(https://github.com/bitcoin/bitcoin/pull/26720#issuecomment-1496413288)
Thanks all for the feedback :). [Update Diff](https://github.com/bitcoin/bitcoin/compare/ba7970c793f3e8ddb54a02256e7ae52e88e47f08..488dbeab56b2cd62e381883e5fe347b446ae7d2c):

* Renamed `max_weight` to `max_inputs_weight`.
* Deduced change size from the max allowed weight for Knapsack and SRD (not from BnB as it's specialized to not create change).
* Adapted doxygen comment to current sources.

💬 RandyMcMillan commented on pull request "build: remove ancient unused define":
(https://github.com/bitcoin/bitcoin/pull/27420#issuecomment-1496440854)
utACK 9fbc5fc

💬 RandyMcMillan commented on pull request "doc: Add example of how to mix private and public keys in descriptors":
(https://github.com/bitcoin/bitcoin/pull/27414#issuecomment-1496446116)
Concept ACK

💬 hebasto commented on pull request "ci: Remove second user account":
(https://github.com/bitcoin/bitcoin/pull/27376#issuecomment-1496449588)
Unfortunately, I've noticed a regression introduced in this PR.

On Ubuntu 22.04, running `FILE_ENV="./ci/test/00_setup_env_native_tidy.sh" ./ci/test_run_all.sh` locally fails with
```
...
IWYU edited 30 files on your behalf.

warning: Not a git repository. Use --no-index to compare two paths outside a working tree
usage: git diff --no-index [<options>] <path> <path>
...
```

👍 hebasto approved a pull request: "compat: move (win) S_* defines into bdb"
(https://github.com/bitcoin/bitcoin/pull/26832)
ACK 54e406118926c2a526455a60f67961520dfb65da, I have reviewed the code and it looks OK.

nit: Two commits _"refactor: don't avoid sys/types.h on when building for Windows"_ and _"compat: move (win) S\_* defines into bdb"_ were squashed but the commit message of the latter was lost.

👍 Ayush170-Future approved a pull request: "build: remove ancient unused define"
(https://github.com/bitcoin/bitcoin/pull/27420)
ACK