💬 josibake commented on pull request "ci: use clang-16 in tidy task":
(https://github.com/bitcoin/bitcoin/pull/27404#issuecomment-1497365274)
ACK https://github.com/bitcoin/bitcoin/pull/27404/commits/a56c96507a9e943bbcd7e126bc827de9495f0ebd
> Included this. I think we could merge this as-is, but should also figure out why it's needed. It feels like there is another issue here.
did a little digging and found this: https://github.com/include-what-you-use/include-what-you-use/issues/679#issuecomment-482798379 , which seems to still be an issue based on the discussion in the linked solution. fwiw, the fix you have here seems to be o
...
(https://github.com/bitcoin/bitcoin/pull/27404#issuecomment-1497365274)
ACK https://github.com/bitcoin/bitcoin/pull/27404/commits/a56c96507a9e943bbcd7e126bc827de9495f0ebd
> Included this. I think we could merge this as-is, but should also figure out why it's needed. It feels like there is another issue here.
did a little digging and found this: https://github.com/include-what-you-use/include-what-you-use/issues/679#issuecomment-482798379 , which seems to still be an issue based on the discussion in the linked solution. fwiw, the fix you have here seems to be o
...
👍 hebasto approved a pull request: "ci: fix git dubious permissions error"
(https://github.com/bitcoin/bitcoin/pull/27423)
ACK 2c11e94131447bc605b7698a80d02c2523497a4b, tested on Ubuntu 22.04.
(https://github.com/bitcoin/bitcoin/pull/27423)
ACK 2c11e94131447bc605b7698a80d02c2523497a4b, tested on Ubuntu 22.04.
💬 hebasto commented on pull request "ci: fix git dubious permissions error":
(https://github.com/bitcoin/bitcoin/pull/27423#discussion_r1158438615)
A stray `#`?
(https://github.com/bitcoin/bitcoin/pull/27423#discussion_r1158438615)
A stray `#`?
💬 josibake commented on pull request "ci: fix git dubious permissions error":
(https://github.com/bitcoin/bitcoin/pull/27423#discussion_r1158448194)
fixed
(https://github.com/bitcoin/bitcoin/pull/27423#discussion_r1158448194)
fixed
💬 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_r1158456950)
yep thanks, fixed
(https://github.com/bitcoin/bitcoin/pull/26720#discussion_r1158456950)
yep thanks, fixed
👍 hebasto approved a pull request: "ci: fix git dubious permissions error"
(https://github.com/bitcoin/bitcoin/pull/27423)
re-ACK ed4a8339b8fe796b4668e206d7fb9c2b120f8eb2
(https://github.com/bitcoin/bitcoin/pull/27423)
re-ACK ed4a8339b8fe796b4668e206d7fb9c2b120f8eb2
💬 fanquake commented on pull request "build: Pointer Authentication and Branch Target Identification for aarch64 Linux (Guix)":
(https://github.com/bitcoin/bitcoin/pull/24123#issuecomment-1497434168)
Rebased past #27406.
Note that the branch protection option being added to libevent here, can now exist inside the `NO_HARDEN` clause.
(https://github.com/bitcoin/bitcoin/pull/24123#issuecomment-1497434168)
Rebased past #27406.
Note that the branch protection option being added to libevent here, can now exist inside the `NO_HARDEN` clause.
🚀 fanquake merged a pull request: "ci: use clang-16 in tidy task"
(https://github.com/bitcoin/bitcoin/pull/27404)
(https://github.com/bitcoin/bitcoin/pull/27404)
💬 fanquake commented on pull request "contrib: allow multi-sig binary verification v2":
(https://github.com/bitcoin/bitcoin/pull/27358#discussion_r1158537830)
`--no-trust-builder-keys` is no-longer a thing? Arguments here need re-ordering as well.
(https://github.com/bitcoin/bitcoin/pull/27358#discussion_r1158537830)
`--no-trust-builder-keys` is no-longer a thing? Arguments here need re-ordering as well.
💬 fanquake commented on pull request "contrib: allow multi-sig binary verification v2":
(https://github.com/bitcoin/bitcoin/pull/27358#discussion_r1158526427)
Can drop `VERSIONPREFIX` from this output.
(https://github.com/bitcoin/bitcoin/pull/27358#discussion_r1158526427)
Can drop `VERSIONPREFIX` from this output.
💬 fanquake commented on pull request "contrib: allow multi-sig binary verification v2":
(https://github.com/bitcoin/bitcoin/pull/27358#discussion_r1158530776)
note that the keyserver we currently "recommend" is `hkps://keys.openpgp.org`. See #22688, #23466.
(https://github.com/bitcoin/bitcoin/pull/27358#discussion_r1158530776)
note that the keyserver we currently "recommend" is `hkps://keys.openpgp.org`. See #22688, #23466.
💬 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
```
(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.
(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`
...
(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
...
(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.
(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`.
(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.
(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
(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
...
(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
...