💬 Mob1le commented on pull request "fuzz: Remove legacy int parse fuzz tests":
(https://github.com/bitcoin/bitcoin/pull/27344#discussion_r1152829942)
WTF

💬 S3RK commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1152831466)
I meant, that we can write only the key for `best_xpub`. Why do we need to write all the candidates as well?

💬 S3RK commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#issuecomment-1489825161)
Approach ACK.

Doing a proper code review now. Have a question about the upgrade. What do you think we should do in the following situation?

- Create a blank wallet with master branch
- import 6 descriptors derived from the same key. `WALLET_FLAG_GLOBAL_HD_KEY` is not set and no master key
- `getxpub` reports "This wallet does not have an active xpub"
- reload the wallet. Upgrade code is triggered, it sets the flag and the master key

This will become even more confusing when/if we la
...

💬 josibake commented on pull request "test: fix intermittent issue in `feature_bip68_sequence`":
(https://github.com/bitcoin/bitcoin/pull/27177#issuecomment-1489830936)
re-ACK https://github.com/bitcoin/bitcoin/commit/f8ebcb823fc5f39adc87ea6cf079f3e72ee44f30

💬 vasild commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1152902779)
It is not possible to pass `nullptr` as an argument to `GetQueryParameterFromUri(const evhttp_uri& uri_parsed, const std::string& key)`. This is the point. Inside the function there is no need to worry about null pointer.

The crash occurs inside the test:

```cpp
auto uri_parsed{evhttp_uri_parse(uri.data())}; // ends up nullptr
auto param_value{GetQueryParameterFromUri(*uri_parsed, key)}; // crash here, before calling GetQueryParameterFromUri().
```

The function is safe, the c
...

💬 vasild commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1152924472)
I see you have adjusted the test in the latest push, I think that is the right approach. Thanks!

💬 hebasto commented on pull request "guix: use python-minimal (3.9)":
(https://github.com/bitcoin/bitcoin/pull/27361#issuecomment-1489947011)
Concept ACK.

💬 hebasto commented on issue "gen-manpages output depends on build options, so needs to check them":
(https://github.com/bitcoin/bitcoin/issues/17506#issuecomment-1489973203)
> wanted to know whether this issue is still active as i am interested in this issue..

Yes. The issue is still unresolved.

📝 fanquake locked a pull request: "Added clangd .cache and compile_commands.json to .gitignore"
(https://github.com/bitcoin/bitcoin/pull/27329)
Added cause these were always getting created whenever I was using clangd in the project

💬 0xB10C commented on pull request "Remove -mempoolfullrbf option":
(https://github.com/bitcoin/bitcoin/pull/26525#issuecomment-1490007597)
> Anyway the data from [miningpool.observer/template-and-block](https://miningpool.observer/template-and-block) shows that blocks routinely include a dozen or so unexpected transactions. It's not clear if those are transactions not in miningpool.observer's mempoo, and thus relevant to compact blocksl; @0xB10C, can you clarify this point?

Transactions listed under "Extra Transactions" are transactions that weren't in my mempool but the pool knew about. These are transactions my node would have
...

:lock: fanquake locked an issue: "Bitcoin-core. MacOs"
(https://github.com/bitcoin/bitcoin/issues/27193)

💬 vasild commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#issuecomment-1490015098)
Since the `replySent` parameter of the `HTTPRequest` constructor is used only from the tests, I would suggest to remove it and adjust the test as needed. I believe the tests should be testing the same API that is used by the real code, otherwise they may end up testing scenarios that never happen in real code and missing real code scenarios.

Furthermore that `replySent = true` from the test is a hack to workaround another problem. It is actually a lie - the reply has not been sent. The proble
...

📝 hebasto opened a pull request: "refactor: Drop no longer used `CNetMsgMaker` instances"
(https://github.com/bitcoin/bitcoin/pull/27368)
The removed lines have been unused since the https://github.com/bitcoin/bitcoin/commit/abf5d16c24cb08b0451bdbd4d1de63a12930e8f5 commit from https://github.com/bitcoin/bitcoin/pull/25454.

💬 Sjors commented on pull request "refactor: remove unused param from legacy pubkey interface":
(https://github.com/bitcoin/bitcoin/pull/27274#discussion_r1153050139)
I'm guessing that just dropping the space will fix the CI issue?

```cpp
/*fRequestedInternal=*/false
```

💬 Sjors commented on pull request "Ignore datacarrier limits for dataless OP_RETURN outputs":
(https://github.com/bitcoin/bitcoin/pull/27261#issuecomment-1490063128)
> bare OP_RETURN outputs are not data carrying outputs

Agreed. But if you stick to this approach, please write a test.

💬 dergoegge commented on pull request "ci: use LLVM/clang-16 in native_fuzz (ASAN) job":
(https://github.com/bitcoin/bitcoin/pull/27363#issuecomment-1490075350)
utACK a634c288c34627eb4ea6f919c99339256f490ede

👍 dergoegge approved a pull request: "ci: use LLVM/clang-16 in native_fuzz (ASAN) job"
(https://github.com/bitcoin/bitcoin/pull/27363)

💬 darosior commented on issue "`listtransactions` fails to list self-send transactions (for imported descriptor wallet)":
(https://github.com/bitcoin/bitcoin/issues/26096#issuecomment-1490083897)
@msgilligan any luck with your testing?

💬 Bushstar commented on pull request "refactor: remove unused param from legacy pubkey interface":
(https://github.com/bitcoin/bitcoin/pull/27274#discussion_r1153075085)
CI is passing now after rename `internal` to `fRequestedInternal`. CI checks below appear all green to me.

💬 dergoegge commented on pull request "refactor: Drop no longer used `CNetMsgMaker` instances":
(https://github.com/bitcoin/bitcoin/pull/27368#issuecomment-1490101331)
utACK ea7ec7808745805c0a18513d7da271dedb2de3f1