💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1608327017)
_from the main thread at https://github.com/bitcoin/bitcoin/pull/29415#pullrequestreview-2041604763_

> The main thing I'm wondering currently is why are we tacking `m_private_broadcast_connections_to_open` loosely? It feels harder to reason about, but I don't see what the benefit of it is.

The reason for this is that it is simpler to implement that way (I think, somebody has a simpler proposal?). This is because the connman thread is creating the connections, they are consumed/used by the
...

💬 hebasto commented on pull request "build: Assume HAVE_CONFIG_H, Add IWYU pragma keep to bitcoin-config.h includes":
(https://github.com/bitcoin/bitcoin/pull/29494#issuecomment-2122679965)
Ported to the CMake-based build system in https://github.com/hebasto/bitcoin/pull/203.

💬 furszy commented on pull request "test: improve robustness of connect_nodes()":
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1608374619)
> I guess this would mean slightly cleaner code, but would make it harder to see which part of the single function returned False when a failure happens. Though, debug logs could be added to help with that, if needed.
>
> What do you think?

I don't think that the slightly cleaner code worth the extra debug logs noise we will get. These will be outputted even when peers connect successfully, so we would see some "connect_nodes(): veracity msg hasn't arrived yet" or ".. pong msg hasn't arrive
...

💬 fanquake commented on pull request "build: Remove `--enable-threadlocal`":
(https://github.com/bitcoin/bitcoin/pull/30137#issuecomment-2122701550)
Guix Build (aarch64):
```bash
77a67096a2225cafa44637d69e9618cb275b26eeda00585dfa246dfbec7fc818 guix-build-17fe948cce2e/output/aarch64-linux-gnu/SHA256SUMS.part
57867960d6c8597648cc49efe4165e543b1181ad0a3a97a16f66bd41b93afe8b guix-build-17fe948cce2e/output/aarch64-linux-gnu/bitcoin-17fe948cce2e-aarch64-linux-gnu-debug.tar.gz
76c872bcb98e8fc46a5cb2ebd13c489caa6b7d3b0b6db92c467aae27b30308fc guix-build-17fe948cce2e/output/aarch64-linux-gnu/bitcoin-17fe948cce2e-aarch64-linux-gnu.tar.gz
5093f7
...

💬 fanquake commented on pull request "build: LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-2122705861)
> Looks like there is a mismatch on the arm64 build.

Disabled adhoc-codesigning for now, as the non-determinism is coming from the identifier field (also why it only happens for arm64). also cc @theuni

💬 furszy commented on pull request "test: improve robustness of connect_nodes()":
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1608389058)
sure. Pushed.

🤔 glozow reviewed a pull request: "refactor prep for package rbf"
(https://github.com/bitcoin/bitcoin/pull/30072#pullrequestreview-2068674969)
lgtm ACK 6fcd3cc016eaf07e47c5c448482ff844855a247b

💬 glozow commented on pull request "refactor prep for package rbf":
(https://github.com/bitcoin/bitcoin/pull/30072#discussion_r1608351999)
07bb2736c17dd50b6ec3d4770001f8e6f0c8710a: would be good to put the sanity checks here

```
// If we are using package feerates, we must be doing package submission.
// It also means carveouts and sibling eviction are not permitted.
if (m_package_feerates) {
Assume(m_package_submission);
Assume(!m_allow_carveouts);
Assume(!m_allow_sibling_eviction);
}
if (m_allow_sibling_evction)
...

💬 glozow commented on pull request "policy: restrict all TRUC (v3) transactions to 10KvB":
(https://github.com/bitcoin/bitcoin/pull/29873#discussion_r1608401645)
done

💬 glozow commented on pull request "policy: restrict all TRUC (v3) transactions to 10KvB":
(https://github.com/bitcoin/bitcoin/pull/29873#discussion_r1608401789)
done

💬 glozow commented on pull request "policy: restrict all TRUC (v3) transactions to 10kvB":
(https://github.com/bitcoin/bitcoin/pull/29873#issuecomment-2122725600)
Thanks @murchandamus, addressed nits

💬 pablomartin4btc commented on pull request "assumeutxo, rpc: Improve EOF error when reading snapshot metadata in loadtxoutset":
(https://github.com/bitcoin/bitcoin/pull/28670#issuecomment-2122739997)
> Hm, unfortunately, I think this PR was made redundant by #29612 recently. After the latest feedback there which I addressed just 3 weeks ago, we are enhancing the assumeutxo metadata there significantly and this means all of the lines changed here are touched there as well and I don't see much of an overlap either that would let me use the code here.

No prob, thanks for letting me know, I'll review that PR.

✅ pablomartin4btc closed a pull request: "assumeutxo, rpc: Improve EOF error when reading snapshot metadata in loadtxoutset"
(https://github.com/bitcoin/bitcoin/pull/28670)

💬 pablomartin4btc commented on pull request "assumeutxo, rpc: Improve EOF error when reading snapshot metadata in loadtxoutset":
(https://github.com/bitcoin/bitcoin/pull/28670#issuecomment-2122741411)
> Hm, unfortunately, I think this PR was made redundant by #29612 recently. After the latest feedback there which I addressed just 3 weeks ago, we are enhancing the assumeutxo metadata there significantly and this means all of the lines changed here are touched there as well and I don't see much of an overlap either that would let me use the code here.

No prob, thanks for letting me know, I'll review that PR.

💬 theuni commented on pull request "util: avoid using thread_local variable that has a destructor":
(https://github.com/bitcoin/bitcoin/pull/30095#discussion_r1608423170)
That'd be my preference, yes. I'll see about adding a clang-tidy check for that combo.

📝 furszy reopened a pull request: "wallet: re-activate the not triggered "AmountWithFeeExceedsBalance" error"
(https://github.com/bitcoin/bitcoin/pull/25269)
Please review #25806 first.

Came up during a talk with @theStack: the `AmountWithFeeExceedsBalance` error inside `WalletModel:prepareTransaction` is never been thrown.

The explanation for it: `createTransaction` does not retrieve the fee if the process fail for insufficient funds (fee return argument is set only at the bottom of the process, when the transaction creation success). So, if the tx creation fails, it's not available inside `WalletModel::prepareTransaction` to be able to throw
...

👋 furszy's pull request is ready for review: "wallet: re-activate the not triggered "AmountWithFeeExceedsBalance" error"
(https://github.com/bitcoin/bitcoin/pull/25269)

💬 maflcko commented on pull request "util: avoid using thread_local variable that has a destructor":
(https://github.com/bitcoin/bitcoin/pull/30095#discussion_r1608448187)
> clang-tidy check

A faster and easier approximation would be to use `git grep -l thread_local`, excluding this file (threadnames.cpp), and the normal lint-exclude list.

💬 stickies-v commented on pull request "indexes: Don't wipe indexes again when continuing a prior reindex":
(https://github.com/bitcoin/bitcoin/pull/30132#issuecomment-2122786225)
Approach ACK. First 2 commits (9de8b263dabd6dd2f86f1f0253c6ee3fac7a4407) LGTM but the third one I'm going to need to spend a lot more time wrapping my head around the implications.

💬 adityajatnika commented on pull request "ci: Add mising -Wno-error=maybe-uninitialized to armhf task":
(https://github.com/bitcoin/bitcoin/pull/30144#issuecomment-2122791047)
lgtm