💬 fanquake commented on pull request "contrib: allow multi-sig binary verification v2":
(https://github.com/bitcoin/bitcoin/pull/27358#discussion_r1158540308)
```suggestion
--min-good-sigs 10
```

💬 fanquake commented on pull request "build: produce a .zip for macOS distribution":
(https://github.com/bitcoin/bitcoin/pull/27099#issuecomment-1497547148)
Rebased, and added a number of small doc fixups.

💬 EthanHeilman commented on pull request "Fixes compile errors in MSVC build #27332":
(https://github.com/bitcoin/bitcoin/pull/27335#discussion_r1158582475)
I think that is a fair point. The issue is that if we ever need to upgrade libevent due to security vulnerability it will break and you'll have to figure this out again and change `build_msvc/bitcoin_config.h.in`. This addresses that issue today rather then at some point in the future. That said, if we expect to move to CMake soon, then it makes sense to move to the previous version and get the minimum diff.

I'll change this to `2.1.12#7`, but I'm open to changing to back to `2.1.12+20230128`
...

📝 fanquake converted_to_draft a pull request: "build: LLVM 16 & LLD based macOS toolchain"
(https://github.com/bitcoin/bitcoin/pull/21778)
This switches us to using a [LLVM](https://llvm.org/) & [LLD](https://lld.llvm.org/) based toolchain for macOS builds. Much work has recently been done on a [new Mach-O backend](https://reviews.llvm.org/rG03f43b3aca3) for LLD and as of LLVM 13 it has become [the default backend](https://reviews.llvm.org/D95204).

With this change I'm able to everything compile and link `bitcoind` and utilities. `bitcoin-qt` and `libbitcoinconsensus` don't currently build. The binaries produced run OK when tak
...

💬 jonatack commented on pull request "test: create random and coins utils, add amount helper, dedupe add_coin":
(https://github.com/bitcoin/bitcoin/pull/26940#issuecomment-1497607769)
undefined@glozow Thanks for bringing it up! The rationale was in this thread: https://github.com/bitcoin/bitcoin/pull/26940#discussion_r1083532109. I noticed this as well, and that `src/test/util/chainstate.h` has the same dependency, and made a patch to remove those. Let me see if I can dig it up.

👍 pablomartin4btc approved a pull request: "wallet: finish addressbook encapsulation"
(https://github.com/bitcoin/bitcoin/pull/26836)
undefinedtested ACK.
Performed manual testing using `bitcoin-qt`.

💬 theuni commented on pull request "contrib: allow multi-sig binary verification v2":
(https://github.com/bitcoin/bitcoin/pull/27358#issuecomment-1497743020)
undefinedThanks for the review @fanquake, comments addressed.

LMK if you'd like me to squash. I could either squash mine into the original authors' or mine all into one "fixup" on top of theirs.

💬 pinheadmz commented on pull request "Add wallet method to detect if a key is "active"":
(https://github.com/bitcoin/bitcoin/pull/27216#issuecomment-1497746671)
undefinedrebased on master and addressed some nits from follow-up PR https://github.com/bitcoin-core/gui/pull/723

💬 fanquake commented on issue "index: ThreadSanitizer: data race on vptr ":
(https://github.com/bitcoin/bitcoin/issues/27355#issuecomment-1497754148)
undefinedRetesting this on master (04595484d97100a50c146e5fc080319d9e0f5ca4). 1 line missing from the above output, which is dumped out in the test failure output before the test is re-run is `libc++abi: Pure virtual function called!`:
```bash
2023-04-05T15:47:47.686929Z (mocktime: 2020-08-31T15:34:12Z) [basic block filter index] [util/thread.cpp:20] [TraceThread] basic block filter index thread start
test/blockfilter_index_tests.cpp(147): fatal error: in "blockfilter_index_tests/blockfilter_index_ini
...

💬 fanquake commented on pull request "contrib: allow multi-sig binary verification v2":
(https://github.com/bitcoin/bitcoin/pull/27358#issuecomment-1497757188)
undefined@theuni I don't particularly mind. I think the commits here look ok as-is. If you want to squash them all down into a fixup, that's probably also ok, but going to be a bit messy. Squashing all the changes back into the original commits is probably going to be more messy again.

💬 brunoerg commented on pull request "logging, net: add ASN from peers on logs":
(https://github.com/bitcoin/bitcoin/pull/27412#issuecomment-1497786231)
undefined> We can clean up the documentation and/or behavior of -logips in another PR.

For sure, I can work on that after this one.

💬 EthanHeilman commented on pull request "Fixes compile errors in MSVC build #27332":
(https://github.com/bitcoin/bitcoin/pull/27335#issuecomment-1497789875)
undefinedPushed this on my lunch break. With @hebasto's help this is now a single file, 8 line change. =)

💬 EthanHeilman commented on pull request "Fixes compile errors in MSVC build #27332":
(https://github.com/bitcoin/bitcoin/pull/27335#discussion_r1158760104)
undefinedWith `2.1.12#7` we don't need to include `bcrypt.lib` as an additional dependency and so I have removed it. If we upgrade libevent we will need to add this.

👍 ryanofsky approved a pull request: "refactor: Extract common/args from util/system"
(https://github.com/bitcoin/bitcoin/pull/27419)
undefinedCode review ACK c02e451882bb1d220b944af3b444d4b529ac351a

Just an option to consider, but I think could be good to move `GetConfigFile`, `GetConfigOptions`, `ArgsManager::ReadConfigStream`, and `ArgsManager::ReadConfigFiles` functions to `src/common/config.cpp` instead of `src/common/args.cpp`, since in longer run, it would be nice if it would be nice if there were a `src/common/settings.cpp` file to parse the settings file, `src/common/config.cpp` to parse the config file, and `src/common/arg
...

💬 ryanofsky commented on pull request "refactor: Extract common/args from util/system":
(https://github.com/bitcoin/bitcoin/pull/27419#discussion_r1158754838)
undefinedIn commit "refactor: Extract common/args from util/system" (c02e451882bb1d220b944af3b444d4b529ac351a)

Would be good to drop the coment above line 7 "Server/client environment: argument handling, config file parsing"

💬 mzumsande commented on pull request "indexes: Read the locator's top block during init, allow interaction with reindex-chainstate":
(https://github.com/bitcoin/bitcoin/pull/25193#issuecomment-1497845534)
undefined0577bd6d1bcbbc808fd20d7a368a562799ceccc3 to 974140f9e721740f857b45d10d7dbab62fdbbe53: rebased due to conflict with #25781

💬 pinheadmz commented on pull request "Add warnings for non-active addresses in receive tab and address book":
(https://github.com/bitcoin-core/gui/pull/723#discussion_r1158814308)
undefinedYeah the warning is the same, but in the GUI we have these related-but-different classes `RecentRequestEntry` for one view and `AddressTableEntry` for another. I don't see a great common file in `qt/` to declare warning strings, so I'm open to suggestions or maybe I should just start a new file?

💬 pinheadmz commented on pull request "Add warnings for non-active addresses in receive tab and address book":
(https://github.com/bitcoin-core/gui/pull/723#issuecomment-1497872600)
undefinedThanks for all the style notes @jonatack I addressed all your comments including adding a new struct for `ReceiveRequest` and renaming/qualifying the new method to `[[nodiscard]] GetAddressWarnings()`. Also addresed your nit in https://github.com/bitcoin/bitcoin/pull/27216 and rebased both PRs on master.

💬 pinheadmz commented on pull request "Add warnings for non-active addresses in receive tab and address book":
(https://github.com/bitcoin-core/gui/pull/723#issuecomment-1497880274)
undefinedI also moved the warnings column to the right, after date. I think we could just remove the title "warnings" and that column would narrow down to 1 character ...?


👍 hebasto approved a pull request: "Fixes compile errors in MSVC build #27332"
(https://github.com/bitcoin/bitcoin/pull/27335)
undefinedACK c40c9edf9266bac0b3553a3d6c5b97f94c34312d