💬 vasild commented on pull request "p2p: cleanup `LookupIntern`, `Lookup` and `LookupHost`":
(https://github.com/bitcoin/bitcoin/pull/26261#discussion_r1139178852)
This is removed in its own commit
```
p2p: remove duplicated `ContainsNoNUL` calls in `Lookup`
```
but I guess that removal can be squashed into one of the preceding commits:
```
p2p, refactor: return vector/optional<CService> in `Lookup`
```
like the same check is removed from `LookupHost()` in
```
p2p, refactor: return `std::optional<CNetAddr>` in `LookupHost`
```

💬 stickies-v commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1139143140)
nit
```suggestion
if (!m_uri_parsed) return std::nullopt;
```

💬 stickies-v commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1139180604)
See https://github.com/bitcoin/bitcoin/pull/24012 - I couldn't get mocking an `evhttp_request*` past the ASan test. That's why in #24098 I introduced the `GetQueryParameterFromUri()` helper function: it could easily be unit tested, and then `HTTPRequest::GetQueryParameter()` was just a simple wrapper left untested.

💬 stickies-v commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1139181920)
Seems like unnecessary churn imo, and redefining HTTP status codes in a single functional test doesn't seem like a productive approach. Would leave this out?

💬 mhmdawnallah commented on issue "test: use-of-uninitialized-value in sqlite3Strlen30":
(https://github.com/bitcoin/bitcoin/issues/27222#issuecomment-1472533007)
Hi @MarcoFalke, I'd like to work on this issue. Could you please let me know where I should start ?

💬 achow101 commented on pull request "test: psbt: check non-witness UTXO removal for segwit v1 input":
(https://github.com/bitcoin/bitcoin/pull/27200#issuecomment-1472543537)
ACK 3dd2f6461b4bb28b2b212c691a3df28ac793ad91

👍 pablomartin4btc approved a pull request: "Wallet : Allow user to navigate options while encrypting at creation"
(https://github.com/bitcoin-core/gui/pull/722)
Tested ACK.
It works as expected according to the description of the PR and it fulfils the requirements of the related issue.
Just as a reminder for other reviewers, if you want to [verify if the wallet was encrypted](https://bitcoin.stackexchange.com/questions/101664/why-is-bitcoin-core-not-asking-me-to-enter-the-decryption-key-for-my-allegedly) or [not](https://bitcoin.stackexchange.com/questions/101664/why-is-bitcoin-core-not-asking-me-to-enter-the-decryption-key-for-my-allegedly), when a u
...

🚀 achow101 merged a pull request: "test: psbt: check non-witness UTXO removal for segwit v1 input"
(https://github.com/bitcoin/bitcoin/pull/27200)

💬 MarcoFalke commented on issue "test: use-of-uninitialized-value in sqlite3Strlen30":
(https://github.com/bitcoin/bitcoin/issues/27222#issuecomment-1472581735)
I am not sure. This was an intermittent issue, so it may be:

* A bug/race in our code
* A bug/race in sqlite
* A bug/race in msan
* A bug/race in the CI env
* A bug/race somewhere else
* A cosmic ray

📝 instagibbs opened a pull request: "Fix fund transaction case at 0-value, 0-fee"
(https://github.com/bitcoin/bitcoin/pull/27271)
and when no inputs are pre-selected.

triggered via:

walletcreatefundedpsbt '[]' '[{"data": "deadbeef"}]' 0 '{"fee_rate": "0"}'

💬 0xB10C commented on pull request "doc: fix typo in interface_usdt_utxocache.py":
(https://github.com/bitcoin/bitcoin/pull/27268#issuecomment-1472641877)
ACK

💬 pablomartin4btc commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1139308077)
thanks, corrected other similar 2.

💬 pablomartin4btc commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1139309346)
I'm now reusing the HTTPStatus enum from the python http module in python; there were like 20 places where these codes were introduced as int. I think it makes it clearer but pls let me know if that's not the case.

💬 Xekyo commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#issuecomment-1472679470)
@theStack: Thanks for the suggestion. I tried implementing this and found that it does save the six lines here, but it also required special casing two places downstream in my follow-up PR. While the fixes were simple, I feel that it makes the interface slightly worse for the downstream consumer, so I’m ambivalent on the change.

💬 pablomartin4btc commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1139311306)
If I bring it back I won't be able to access to m_uri_parsed from the helper function, unless there's another alternative, I'll have to roll-back to the original approach of the PR.

💬 stickies-v commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1139348308)
Every line of code that you change needs review. If those changed lines of code are unrelated to what you're trying to achieve, you risk unnecessarily slowing doing the PR or even getting NACKs for reasons unrelated to what you're really trying to do. I'd strongly suggest keeping the scope as small as possible.

I'm not saying it's necessarily a bad idea to use enumerated HTTP status codes, but it would be better off in a separate PR/issue (and have you checked if it's been suggested before?)

💬 BitcoinErrorLog commented on pull request "Remove -mempoolfullrbf option":
(https://github.com/bitcoin/bitcoin/pull/26525#issuecomment-1472729383)
Just reviving this. Four months have passed. What are the current stats on people flagging mempoolfullrbf=1?

My guess is that literally only the hostile devs in this thread run it, and whichever miners they managed to persuade personally, and that there is no significant signal of support or usage of the feature in the wild, or am I wrong?

It would be nice for the remaining thousands of users and merchants to not have to look over our shoulder and wonder if Core will sneak it on by defaul
...

💬 fanquake commented on pull request "p2p: Improve diversification of new connections":
(https://github.com/bitcoin/bitcoin/pull/27264#issuecomment-1472730802)
cc @naumenkogs @amitiuttarwar @sdaftuar

💬 fanquake commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#issuecomment-1472733580)
```bash
test 2023-03-16T20:06:03.567000Z TestFramework (ERROR): Unexpected exception caught during testing
Traceback (most recent call last):
File "/private/var/folders/v7/fs2b0v3s0lz1n57gj9y4xb5m0000gn/T/cirrus-ci-build/ci/scratch/build/bitcoin-arm64-apple-darwin/test/functional/test_framework/test_framework.py", line 132, in main
self.run_test()

...

💬 fanquake commented on pull request "doc: fix typo in interface_usdt_utxocache.py":
(https://github.com/bitcoin/bitcoin/pull/27268#issuecomment-1472741835)
Can you update the commit message to the same as what I've changed the PR title too: `doc: fix typo in interface_usdt_utxocache.py`.