💬 Sjors commented on pull request "Switch hardened derivation marker to h (in normalized descriptors and new wallets)":
(https://github.com/bitcoin/bitcoin/pull/26076#discussion_r1157435639)
I modified the text a bit, and also updated the PR description.

💬 Sjors commented on pull request "Switch hardened derivation marker to h (in normalized descriptors and new wallets)":
(https://github.com/bitcoin/bitcoin/pull/26076#discussion_r1157439737)
We don't normalize the private key version of descriptors in general. I'm not sure if it matters a lot, but would prefer to keep such a change to followup.

💬 Xekyo 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_r1157441154)
I’m confused, @aureleoules’s suggestion makes sense to me. If it’s the maximum allowed transaction weight, why is it reduced by the `tx_noinputs_size`? If it’s supposed to be the allowance for inputs, it might additionally need to be reduced by an allowance for the change output?

💬 jonatack commented on pull request "doc: Add example of how to mix private and public keys in descriptors":
(https://github.com/bitcoin/bitcoin/pull/27414#discussion_r1157437268)
```suggestion
steps, for signing transactions, or for dumping wallet descriptors
```

💬 dergoegge commented on pull request "reduce cs_main scope, guard block index 'nFile' under a local mutex":
(https://github.com/bitcoin/bitcoin/pull/27006#issuecomment-1496245186)
> I never said the opposite. Not sure why the "rush" term is being used here really. I haven't said anything like "this is great, we must have it now".. (nor here nor anywhere in the repository).

I'm sorry didn't mean to put words in your mouth. It seemed rushed to me because you are going with an "easier" solution here rather than the more complex but (imo) worth while long term design.

> it will need blockstorage dependency to use the thread analysis annotation

I was thinking of not a
...

💬 Sjors commented on pull request "Switch hardened derivation marker to h (in normalized descriptors and new wallets)":
(https://github.com/bitcoin/bitcoin/pull/26076#discussion_r1157485963)
The bigger break was actually in the wallet dump format. I've added handling to keep `hdkeypath` the same for legacy wallets.

💬 Sjors commented on pull request "Switch hardened derivation marker to h (in normalized descriptors and new wallets)":
(https://github.com/bitcoin/bitcoin/pull/26076#discussion_r1157488214)
Dropped.

💬 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_r1157488656)
> it's the maximum allowed transaction weight, not only for the inputs.
I don't think so? The coin selection algos don't have any knowledge about the additional data of the to-be-created tx (static header size + outputs), so they can only work with a weight limit on the inputs.

oops, I shouldn't answer comments at night. My bad.
it's ok to say that is the max inputs weight. Will rename the variable in the next push.

> If it’s supposed to be the allowance for inputs, it might additionally
...

💬 Sjors commented on pull request "Switch hardened derivation marker to h (in normalized descriptors and new wallets)":
(https://github.com/bitcoin/bitcoin/pull/26076#issuecomment-1496278211)
Biggest change is that the `hdkeypath` field in `getaddressinfo` keep using `'` for legacy wallets. The legacy backup (import) dump format also uses `'`.

💬 jonatack commented on pull request "Fix logging RPC and -debugexclude with 0/none values, add test coverage, improve docs":
(https://github.com/bitcoin/bitcoin/pull/27231#issuecomment-1496283614)
Rebased!

💬 sdaftuar commented on pull request "mempool: disallow txns under min relay fee, even in packages":
(https://github.com/bitcoin/bitcoin/pull/26933#issuecomment-1496286578)
I think in the interests of moving the package relay and v3 project forward, it makes sense to adopt this approach and defer potential improvements like #27018 to the future. My thinking is that this PR is a conservative way forward that seems obviously safe; our eviction logic on the other hand has a number of shortcomings, which I think will be best to fix up first before making more changes to use the eviction logic in new scenarios. We can work on the mempool and eviction in parallel and (ho
...

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