💬 instagibbs commented on pull request "test: tx orphan handling":
(https://github.com/bitcoin/bitcoin/pull/28199#issuecomment-1669753053)
ACK https://github.com/bitcoin/bitcoin/pull/28199/commits/abe8536192c9f2cd6ba9d0e083f23dec4d20841f

https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1283271742 would be nice to be slightly cleaned up for future readers

💬 glozow commented on pull request "test: tx orphan handling":
(https://github.com/bitcoin/bitcoin/pull/28199#issuecomment-1669788993)
thanks for the concept acks @dergoegge @jamesob @brunoerg @Empact @jonatack, would appreciate a review of test too :pray:

💬 fjahr commented on pull request "Add ASM optimizations for MuHash3072":
(https://github.com/bitcoin/bitcoin/pull/19181#issuecomment-1669793604)
> There hasn't been much activity lately. What is the status here?
>
> [Finding reviewers](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#finding-reviewers) may take time. However, if the patch is no longer relevant, please close this pull request. If the author lost interest or time to work on this, please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

Still relevant... How good is your ASM, @DrahtBot ? 😁

💬 fanquake commented on pull request "guix: use GCC 12.3.0 to build releases":
(https://github.com/bitcoin/bitcoin/pull/27897#issuecomment-1669797798)
Upstreamed a patch to (re-)add `powerpc64-linux` as a platform definition in Guix: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=65151. Using a self-compiled Guix with this patch, fixes building our cross-compilation toolchain for `powerpc64-linux-gnu`, and means we'd just drop 26ee1fc5998cce6808f022fd1a4e1764115c2d57 from this PR.

Also pushed a patch to update mingw-w64 to `11.0.1`: https://lists.gnu.org/archive/html/guix-patches/2023-08/msg00299.html.

💬 Sjors commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#issuecomment-1669800569)
Maybe point out in the description how the first commit 8283ca76e4feb37d3514fbd8dc491b8c4965dff1 relates to BIP 324, which doesn't use the ecdh module, but ElligatorSwift instead.

💬 furszy commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1287286613)
Have thought more about this and I'm not so convinced anymore.

The connection manager class lives on a lower level, and manages only a fraction of the node's information. And this field is strictly linked to the node's overall status. Right now, `HasDesirableServiceFlags()` requires to know the chain sync status.
So, by placing it inside `CConMan`, we add another level of indirection. Because, most of the time, the peer manager class will call to the `CConMan` setter and subsequently call th
...

👍 TheCharlatan approved a pull request: "refactor: Remove unused boost signals2 from torcontrol"
(https://github.com/bitcoin/bitcoin/pull/28240#pullrequestreview-1567496979)
ACK fa32af22b323e7c58b6b20af6517f4795a72cdc5

The `async_handler` was unused since its introduction, so I don't think it's worth it to keep it around.

💬 TheCharlatan commented on pull request "refactor: Remove unused boost signals2 from torcontrol":
(https://github.com/bitcoin/bitcoin/pull/28240#discussion_r1287241422)
Style nit: These pragmas could be indented like they are here: https://github.com/bitcoin/bitcoin/blob/master/src/util/strencodings.h#L20 , or did you omit any automatic formatting to maintain order?

💬 TheCharlatan commented on pull request "refactor: Remove unused boost signals2 from torcontrol":
(https://github.com/bitcoin/bitcoin/pull/28240#discussion_r1287296853)
Maybe change this to "These messages could be used to dispatch async notifications to an async handler"?

💬 MarcoFalke commented on pull request "refactor: Remove unused boost signals2 from torcontrol":
(https://github.com/bitcoin/bitcoin/pull/28240#discussion_r1287321938)
I wasn't sure if the includes must come in a specific order. I've pushed a new commit to re-order them, which can be easily reverted, if there are issues.

💬 MarcoFalke commented on pull request "Avoid lock order inversion in `Chainstate::ConnectTip` function":
(https://github.com/bitcoin/bitcoin/pull/27684#issuecomment-1669917794)
Maybe `m_tx_inventory_mutex` can be released before `pool.cs` to break that link instead?

💬 theStack commented on pull request "bench: add readblock benchmark":
(https://github.com/bitcoin/bitcoin/pull/26684#discussion_r1287370102)
Okay, that makes sense. My blind guess was that clang-tidy (or any other similar tool) would only look at the passed expression and not care about the effect of the assert as a whole. Seems like even the passed expression is not side-effect free at several places in the code, e.g.
https://github.com/bitcoin/bitcoin/blob/b565485c24c0feacae559a7f6f7b83d7516ca58d/src/init.cpp#L1428

so it indeed might be better to remove recommendations that we don't even follow ourselves.

(Also, I sadly can'
...

💬 theuni commented on pull request "refactor: Enforce C-str fmt strings in WalletLogPrintf()":
(https://github.com/bitcoin/bitcoin/pull/28237#discussion_r1287382437)
Hmm, this one wasn't being caught before?

🤔 theuni reviewed a pull request: "refactor: Enforce C-str fmt strings in WalletLogPrintf()"
(https://github.com/bitcoin/bitcoin/pull/28237#pullrequestreview-1567718554)
Makes sense to me. I almost did the same thing while poking at `WalletLogPrintf` for the sake of easier static analysis.

Am I missing some obvious reason though, other than "might as well"?

💬 theuni commented on pull request "refactor: Enforce C-str fmt strings in WalletLogPrintf()":
(https://github.com/bitcoin/bitcoin/pull/28237#discussion_r1287387071)
I was going to suggest the `nonnull` attribute here, but I believe our tidy check actually enforces that :)

💬 Sjors commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1287394228)
As long as testnet and signet are the same, and different from mainnet, I'm happy. When testing with e.g. hardware wallets it lets you use the testnet app / firmware instead of signet-specific stuff.

💬 MajesticBank commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1669963434)
#1 At this point I will assume Daniel600 is not just spreading lies but obviously pressured / paid to promote lower privacy on bitcoin blockchain.

There is no service on internet that accept 0-conf bitcoin transaction, end.

I have 10 years experience in this industry and running crypto-instant swap exchange for 2+ years https://majesticbank.sc/

Made over 500,000+ transactions using Coinpayments, Coinspaid and Changelly + fixedfloat, changenow and many more instant-swap sites, no servi
...

💬 Sjors commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1287398956)
37b0ca1c3d753cc61e0debf8ea59160bdf4127ec: alternatively you could scan for the exact pubkey length. That would tolerate future versions to add extra stuff to the address that's safe to ignore for older versions.

💬 Sjors commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1287401010)
37b0ca1c3d753cc61e0debf8ea59160bdf4127ec: could use `version == 0 && silent_payment_data.size() < SILENT_PAYMENT_V0_DATA_SIZE` (see comment above)

💬 Sjors commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1287404996)
328951d056e939b6db573043fc71d8bd6ae68dff: this should be in some bech32 helper function.