💬 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.
(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
...
(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.
(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?
(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"?
(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.
(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?
(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'
...
(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?
(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"?
(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 :)
(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.
(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
...
(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.
(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)
(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.
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1287404996)
328951d056e939b6db573043fc71d8bd6ae68dff: this should be in some bech32 helper function.
💬 Sjors commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1287404315)
328951d056e939b6db573043fc71d8bd6ae68dff: if cheap, could assert/assume both ` CPubKey` are valid.
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1287404315)
328951d056e939b6db573043fc71d8bd6ae68dff: if cheap, could assert/assume both ` CPubKey` are valid.
💬 Sjors commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1287402078)
37b0ca1c3d753cc61e0debf8ea59160bdf4127ec `assert data.size() >= SILENT_PAYMENT_V0_DATA_SIZE` (or early return if you don't expect the caller to have already checked this)
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1287402078)
37b0ca1c3d753cc61e0debf8ea59160bdf4127ec `assert data.size() >= SILENT_PAYMENT_V0_DATA_SIZE` (or early return if you don't expect the caller to have already checked this)
💬 MarcoFalke commented on pull request "refactor: Enforce C-str fmt strings in WalletLogPrintf()":
(https://github.com/bitcoin/bitcoin/pull/28237#discussion_r1287411898)
I hope that the tests (and reviewers) will catch a `LogPrint(nullptr, 1, 2, 3)`. But adding `nonnull` can be done in a follow-up, if needed, for all log-functions.
(https://github.com/bitcoin/bitcoin/pull/28237#discussion_r1287411898)
I hope that the tests (and reviewers) will catch a `LogPrint(nullptr, 1, 2, 3)`. But adding `nonnull` can be done in a follow-up, if needed, for all log-functions.
💬 MarcoFalke commented on pull request "refactor: Enforce C-str fmt strings in WalletLogPrintf()":
(https://github.com/bitcoin/bitcoin/pull/28237#discussion_r1287412409)
Yes, see https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1286821141
(https://github.com/bitcoin/bitcoin/pull/28237#discussion_r1287412409)
Yes, see https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1286821141