💬 brunoerg commented on pull request "p2p: cleanup `LookupIntern`, `Lookup` and `LookupHost`":
(https://github.com/bitcoin/bitcoin/pull/26261#discussion_r1207080551)
The alternative to keep `BOOST_REQUIRE` is creating one more variable but I don't think it's worth, would have to do it in many places. Using `Assert` could also be an alternative, is there any harm of using it? Perhaps import `util/check` just for it?

💬 brunoerg commented on pull request "p2p: cleanup `LookupIntern`, `Lookup` and `LookupHost`":
(https://github.com/bitcoin/bitcoin/pull/26261#discussion_r1207083634)
Since it's a non-blocker I'm pushing addressing all lastest nits, except this one, to not delay and cause futher rebases conflicts.

💬 brunoerg commented on pull request "p2p: cleanup `LookupIntern`, `Lookup` and `LookupHost`":
(https://github.com/bitcoin/bitcoin/pull/26261#issuecomment-1564679459)
Force-pushed addressing:

- https://github.com/bitcoin/bitcoin/pull/26261#discussion_r1201199915
- https://github.com/bitcoin/bitcoin/pull/26261#discussion_r1177587552

Maybe done with nits, and we can have it merged?

👍 stickies-v approved a pull request: "p2p: cleanup `LookupIntern`, `Lookup` and `LookupHost`"
(https://github.com/bitcoin/bitcoin/pull/26261#pullrequestreview-1446643228)
re-ACK 5c832c3820253affc65c0ed490e26e5b0a4d5c9b - just addressing two nits, no other changes

<details>
<summary><code>git range-diff 8b59231641845f71df37e163bf5b8157fb197d05 4eee95e57bb6f773bcaeb405bca949f158a62134 5c832c3820253affc65c0ed490e26e5b0a4d5c9b</code></summary>

```
1: c0cf3d70e = 1: 5c1774a56 p2p, refactor: return `std::vector<CNetAddr>` in `LookupIntern`
2: 94b5edbf6 = 2: 7799eb125 p2p, refactor: return `std::vector<CNetAddr>` in `LookupHost`
3: 617bc481e ! 3: 34bcdfc
...

📝 mmienko opened a pull request: "update osx build docs"
(https://github.com/bitcoin/bitcoin/pull/27769)
- Add Jetbrains IDE dir to gitignore
- Document how to verify Homebrew during OSX build setup

While the project builds for osx M2, developers might not have envrionments properly setup (I didn't becuase I think I installed Homebrew from a guide rather than official site).
Hopefully improved documentation can mitigate future mishaps. Inspired by https://github.com/bitcoin/bitcoin/issues/25556.

💬 glozow commented on pull request "wallet: when a block is disconnected, update transactions that are no longer conflicted":
(https://github.com/bitcoin/bitcoin/pull/27145#issuecomment-1564716873)
> I've fixed a bug which resulted in the conflicted tx not being marked as inactive if the conflicting tx was not in mapTxSpends.

> If mapTxSpends.count(outpoint) == 1, the outpoint was spent only once, so the wallet shouldn't contain any conflicting tx.

I interpreted the bug as = we wouldn't mark a conflicted tx inactive if the block transaction was not in `mapTxSpends` (which would mean count == 1). Normally we'd expect them both to be there (count > 1).

💬 hebasto commented on pull request "update osx build docs":
(https://github.com/bitcoin/bitcoin/pull/27769#discussion_r1207132244)
This should be done in developer's personal environment, not in the repo.

💬 mmienko commented on pull request "update osx build docs":
(https://github.com/bitcoin/bitcoin/pull/27769#discussion_r1207134303)
Ah yeah, there is a global gitignore. Thanks, will revert.

💬 glozow commented on pull request "p2p: Stop relaying non-mempool txs":
(https://github.com/bitcoin/bitcoin/pull/27625#issuecomment-1564726392)
concept ACK

🤔 glozow reviewed a pull request: "wallet: when a block is disconnected, update transactions that are no longer conflicted"
(https://github.com/bitcoin/bitcoin/pull/27145#pullrequestreview-1446696893)
ACK 89df7987c2f1eea42454c2b0efc31a924fbfd3a8

💬 kevkevinpal commented on pull request "rpc: remove deprecated "warning" field from {create,load,restore,unload}wallet":
(https://github.com/bitcoin/bitcoin/pull/27757#issuecomment-1564752809)
ACK [d78f9a7](https://github.com/bitcoin/bitcoin/pull/27757/commits/d78f9a7ed7acf5e2610f0b108123bae1a376f819)

compiled and ran the code below and got the following response
```
--> ./src/bitcoin-cli -regtest -named createwallet wallet_name=oops2 descriptors=false passphrase=""
{
"name": "oops2",
"warnings": [
"Empty string given as passphrase, wallet will not be encrypted.",
"Wallet created successfully. The legacy wallet type is being deprecated and support for creating an
...

💬 fanquake commented on pull request "update osx build docs":
(https://github.com/bitcoin/bitcoin/pull/27769#issuecomment-1564760572)
> (I didn't becuase I think I installed Homebrew from a guide rather than official site).

Concept NACK - Thanks, however steps for how to (re-)install your package manager, are off-topic for these installation instructions, and not something we want to maintain. I'd suggest always reading official (up-to-date) documentation, than random, third-party guides.

💬 pinheadmz commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#issuecomment-1564803705)
@vasild did an overhaul of this PR and implemented more how we discussed on IRC. Edited the description to explain. I ended up not using `std::variant` because I felt it added uneeded complexity. Current approach is `Proxy` just has a `is_unix` flag and abstracts away the `CService` methods like `IsValid()` etc to support TCP or UNIX sockets

💬 stratospher commented on pull request "Introduce secp256k1 module with field and group classes to test framework":
(https://github.com/bitcoin/bitcoin/pull/26222#discussion_r1207054125)
8990668: s/infinite/infinity

💬 stratospher commented on pull request "Introduce secp256k1 module with field and group classes to test framework":
(https://github.com/bitcoin/bitcoin/pull/26222#discussion_r1207054357)
8990668: s/infinite/infinity
maybe make infinity representation in str and repr consistent too.

💬 stratospher commented on pull request "Introduce secp256k1 module with field and group classes to test framework":
(https://github.com/bitcoin/bitcoin/pull/26222#discussion_r1207066672)
8990668: isn't the result updated and not cached here?

💬 stratospher commented on pull request "Introduce secp256k1 module with field and group classes to test framework":
(https://github.com/bitcoin/bitcoin/pull/26222#discussion_r1207067694)
89906684: any particular reason why `v >= FE.SIZE` isn't supported? (have only thought about `ellswift_decode` [scenario](https://github.com/bitcoin/bips/blob/master/bip-0324/ellswift_decode_test_vectors.csv#L77) where v could be greater than `FE.SIZE`)

💬 sipa commented on pull request "Introduce secp256k1 module with field and group classes to test framework":
(https://github.com/bitcoin/bitcoin/pull/26222#discussion_r1207264222)
I don't understand.

💬 sipa commented on pull request "Introduce secp256k1 module with field and group classes to test framework":
(https://github.com/bitcoin/bitcoin/pull/26222#discussion_r1207265123)
__repr__ is supposed to produce a string that is valid Python code.

💬 amitiuttarwar commented on pull request "addrman: select addresses by network follow-up":
(https://github.com/bitcoin/bitcoin/pull/27745#discussion_r1207282099)
good catch! I intended to bundle this assume with the callers, think I accidentally reverted to an in-between state. will fix