💬 hebasto commented on pull request "refactor: throw `std::string_view` instead of `const char*` in constexpr/consteval functions":
(https://github.com/bitcoin/bitcoin/pull/33569#discussion_r2414572764)
If we go down this path, could the linter be assigned to this task as well?
(https://github.com/bitcoin/bitcoin/pull/33569#discussion_r2414572764)
If we go down this path, could the linter be assigned to this task as well?
💬 l0rinc commented on pull request "randomenv: Fix MinGW dllimport warning for `environ`":
(https://github.com/bitcoin/bitcoin/pull/33570#issuecomment-3382585168)
Thanks for the replies, I have updated the description to clarify this is `llvm-mingw` specific, let me know if that's enough.
(https://github.com/bitcoin/bitcoin/pull/33570#issuecomment-3382585168)
Thanks for the replies, I have updated the description to clarify this is `llvm-mingw` specific, let me know if that's enough.
💬 l0rinc commented on pull request "refactor: throw `std::string_view` instead of `const char*` in constexpr/consteval functions":
(https://github.com/bitcoin/bitcoin/pull/33569#discussion_r2414607669)
I also thought of this, it's why I've mentioned https://clang.llvm.org/extra/clang-tidy/checks/misc/throw-by-value-catch-by-reference.html in the descripton in the latest push - but it seems string literal throws are tolerated there.
I don't mind adding such a check, but it seemed to me `contrib/devtools/bitcoin-tidy` isn't actively developed.
(https://github.com/bitcoin/bitcoin/pull/33569#discussion_r2414607669)
I also thought of this, it's why I've mentioned https://clang.llvm.org/extra/clang-tidy/checks/misc/throw-by-value-catch-by-reference.html in the descripton in the latest push - but it seems string literal throws are tolerated there.
I don't mind adding such a check, but it seemed to me `contrib/devtools/bitcoin-tidy` isn't actively developed.
💬 glozow commented on pull request "wallet: don't consider unconfirmed TRUC coins with ancestors":
(https://github.com/bitcoin/bitcoin/pull/33528#issuecomment-3382642331)
> When I went back to test the commands I pasted in https://github.com/bitcoin/bitcoin/issues/33368#issuecomment-3319935660 I found that it was still failing silently despite the functional test failing appropriately.
Nice! This PR is limiting the UTXOs that show up when doing automatic coin selection, and thus errors with "insufficient funds" when it can't find enough UTXOs for the tx. In your test/commands, you're pre-selecting those inputs using `send`.
Btw, when I was testing `send` co
...
(https://github.com/bitcoin/bitcoin/pull/33528#issuecomment-3382642331)
> When I went back to test the commands I pasted in https://github.com/bitcoin/bitcoin/issues/33368#issuecomment-3319935660 I found that it was still failing silently despite the functional test failing appropriately.
Nice! This PR is limiting the UTXOs that show up when doing automatic coin selection, and thus errors with "insufficient funds" when it can't find enough UTXOs for the tx. In your test/commands, you're pre-selecting those inputs using `send`.
Btw, when I was testing `send` co
...
💬 polespinasa commented on pull request "refactor: rename `fees.{h,cpp}` to `fees/block_policy_estimator{h,cpp}`":
(https://github.com/bitcoin/bitcoin/pull/33218#issuecomment-3382650926)
ACK c70781d4241cb8b30fe33e7205868382031b4670
Code reviewed and tested.
These changes are just a refactor and not functional at all, but are needed in order to implement the FeeRate Forecast Manager in a "clean" and "ordered" manner #31664
(https://github.com/bitcoin/bitcoin/pull/33218#issuecomment-3382650926)
ACK c70781d4241cb8b30fe33e7205868382031b4670
Code reviewed and tested.
These changes are just a refactor and not functional at all, but are needed in order to implement the FeeRate Forecast Manager in a "clean" and "ordered" manner #31664
💬 ryanofsky commented on pull request "coins: fix `cachedCoinsUsage` accounting in `CCoinsViewCache`":
(https://github.com/bitcoin/bitcoin/pull/32313#issuecomment-3382685189)
Just a quick code review for 2b11c47f60b85fe72ab9d093c9f8e5f597182fbc. Sorry if I missed anything obvious, my comments below are mostly asking for things to be explained a little better. Overall the changes seem positive and I did not see any problems.
- Can PR description be updated to say what types of "violations" are being "fixed" and clarify what purpose of the changes are? Would be helpful if description was self contained and did not start out referencing a comment in another PR.
-
...
(https://github.com/bitcoin/bitcoin/pull/32313#issuecomment-3382685189)
Just a quick code review for 2b11c47f60b85fe72ab9d093c9f8e5f597182fbc. Sorry if I missed anything obvious, my comments below are mostly asking for things to be explained a little better. Overall the changes seem positive and I did not see any problems.
- Can PR description be updated to say what types of "violations" are being "fixed" and clarify what purpose of the changes are? Would be helpful if description was self contained and did not start out referencing a comment in another PR.
-
...
💬 fanquake commented on pull request "depends: Update URL for `qrencode` package source tarball":
(https://github.com/bitcoin/bitcoin/pull/33494#issuecomment-3382705961)
It's also been pointed out that this has also (effectively) broken the Guix build on the release branches. Prior to this change, any download of the qrencode tarball, was falling back to https://bitcoincore.org/depends-sources. However now that the tarball has been changed (and overwritten on the server), any build for a branch that doesn't have these changes, when it falls back to the server for the download, will fail with a checksum mismatch.
Meaning anyone that tries to do a from scratch
...
(https://github.com/bitcoin/bitcoin/pull/33494#issuecomment-3382705961)
It's also been pointed out that this has also (effectively) broken the Guix build on the release branches. Prior to this change, any download of the qrencode tarball, was falling back to https://bitcoincore.org/depends-sources. However now that the tarball has been changed (and overwritten on the server), any build for a branch that doesn't have these changes, when it falls back to the server for the download, will fail with a checksum mismatch.
Meaning anyone that tries to do a from scratch
...
📝 achow101 opened a pull request: "Revert "depends: Update URL for `qrencode` package source tarball""
(https://github.com/bitcoin/bitcoin/pull/33577)
The new URL breaks CI on the current release branches, see https://github.com/bitcoin/bitcoin/pull/33494#issuecomment-3380802351.
The old URL also no longer exists so the tarball is fetched from the depends sources cache that we host, but the original tarball has already been overwritten on there. We will need to manually reinstate the original tarball.
(https://github.com/bitcoin/bitcoin/pull/33577)
The new URL breaks CI on the current release branches, see https://github.com/bitcoin/bitcoin/pull/33494#issuecomment-3380802351.
The old URL also no longer exists so the tarball is fetched from the depends sources cache that we host, but the original tarball has already been overwritten on there. We will need to manually reinstate the original tarball.
💬 achow101 commented on pull request "depends: Update URL for `qrencode` package source tarball":
(https://github.com/bitcoin/bitcoin/pull/33494#issuecomment-3382716042)
#33577 for the revert
(https://github.com/bitcoin/bitcoin/pull/33494#issuecomment-3382716042)
#33577 for the revert
✅ itornaza closed a pull request: "depends: fix for llvm-ranlib (etc): 'No such file or directory' macOS 15.0"
(https://github.com/bitcoin/bitcoin/pull/30994)
(https://github.com/bitcoin/bitcoin/pull/30994)
💬 janb84 commented on pull request "doc: bump the template macOS version since 14 is now the minimum supported version":
(https://github.com/bitcoin/bitcoin/pull/33573#issuecomment-3382760187)
> > The v30 has still support for 13+. see [https://github.com/bitcoin/bitcoin/pull/33559#discussion_r2414194987`](https://github.com/bitcoin/bitcoin/pull/33559#discussion_r2414194987%60)
> > Think it's still to early to change this if v30 has still full support for that version.
>
> Note that this PR is against master, not 30.x
I get it. but it's changing .github issue templates. Github uses master as a base for the templates right ?
So V30 or not at merge it will use this template as
...
(https://github.com/bitcoin/bitcoin/pull/33573#issuecomment-3382760187)
> > The v30 has still support for 13+. see [https://github.com/bitcoin/bitcoin/pull/33559#discussion_r2414194987`](https://github.com/bitcoin/bitcoin/pull/33559#discussion_r2414194987%60)
> > Think it's still to early to change this if v30 has still full support for that version.
>
> Note that this PR is against master, not 30.x
I get it. but it's changing .github issue templates. Github uses master as a base for the templates right ?
So V30 or not at merge it will use this template as
...
💬 Crypt-iQ commented on pull request "fuzz: compact block harness":
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2414740159)
Answered in the review club, also posting here:
Without this commit, leveldb's `MemTable` is non-deterministic since it depends on insert order. If we sort `vBlocks` right before calling `WriteBatchSync`, then the leveldb non-determinism is solved, but the number of times we call the comparison function varies since the initial ordering before sorting varies across runs of the same input. That means we need to sort when we insert into `m_dirty_blockindex` as the insert order _is_ deterministi
...
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2414740159)
Answered in the review club, also posting here:
Without this commit, leveldb's `MemTable` is non-deterministic since it depends on insert order. If we sort `vBlocks` right before calling `WriteBatchSync`, then the leveldb non-determinism is solved, but the number of times we call the comparison function varies since the initial ordering before sorting varies across runs of the same input. That means we need to sort when we insert into `m_dirty_blockindex` as the insert order _is_ deterministi
...
💬 davidgumberg commented on pull request "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer":
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2414749363)
Thanks for this suggestion, I've taken it.
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2414749363)
Thanks for this suggestion, I've taken it.
💬 davidgumberg commented on pull request "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer":
(https://github.com/bitcoin/bitcoin/pull/32606#issuecomment-3382814452)
Force-pushed to address merge conflict and @hodlinator's feedback to log IP's.
(https://github.com/bitcoin/bitcoin/pull/32606#issuecomment-3382814452)
Force-pushed to address merge conflict and @hodlinator's feedback to log IP's.
💬 glozow commented on pull request "doc: bump the template macOS version since 14 is now the minimum supported version":
(https://github.com/bitcoin/bitcoin/pull/33573#issuecomment-3382815837)
> I get it. but it's changing .github issue templates. Github uses master as a base for the templates right ?
Oh sorry I misunderstood, you're absolutely right. I think it's more likely for people to use v30 on something more modern though.
(https://github.com/bitcoin/bitcoin/pull/33573#issuecomment-3382815837)
> I get it. but it's changing .github issue templates. Github uses master as a base for the templates right ?
Oh sorry I misunderstood, you're absolutely right. I think it's more likely for people to use v30 on something more modern though.
💬 Crypt-iQ commented on pull request "fuzz: compact block harness":
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2414764467)
Will compare the coverage from increasing to 4 vs increasing to 8.
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2414764467)
Will compare the coverage from increasing to 4 vs increasing to 8.
💬 glozow commented on issue "Mempool Expiry eviction might remove txs that could be mined in the next block":
(https://github.com/bitcoin/bitcoin/issues/33510#issuecomment-3382890614)
> Also thanks for digging up the history for expiration; I was wondering if it predated RBF, eviction, and CPFP, but it looks like all those things were added roughly around the same time. I can imagine one other reason for expiration, which probably didn't exist at the time it was added: allowing transactions to be replaced for free (e.g. without the incremental relay fee, as a counter to the pinning that's caused by that).
I'll add that, back in the day, expiry would have been the only way to
...
(https://github.com/bitcoin/bitcoin/issues/33510#issuecomment-3382890614)
> Also thanks for digging up the history for expiration; I was wondering if it predated RBF, eviction, and CPFP, but it looks like all those things were added roughly around the same time. I can imagine one other reason for expiration, which probably didn't exist at the time it was added: allowing transactions to be replaced for free (e.g. without the incremental relay fee, as a counter to the pinning that's caused by that).
I'll add that, back in the day, expiry would have been the only way to
...
💬 theuni commented on pull request "depends: Update URL for `qrencode` package source tarball":
(https://github.com/bitcoin/bitcoin/pull/33494#issuecomment-3382904047)
I don't understand how this was ever intended to work. A depends source has a single hash.. that's pretty fundamental to the way it was designed. And a filename with a new hash is exactly the kind of thing it was intended to catch and disallow.
The filename was changed upstream, and rightly so. Why did we not just adopt the new filename/hash and let that be the end of it?
(https://github.com/bitcoin/bitcoin/pull/33494#issuecomment-3382904047)
I don't understand how this was ever intended to work. A depends source has a single hash.. that's pretty fundamental to the way it was designed. And a filename with a new hash is exactly the kind of thing it was intended to catch and disallow.
The filename was changed upstream, and rightly so. Why did we not just adopt the new filename/hash and let that be the end of it?
💬 hebasto commented on pull request "depends: Update URL for `qrencode` package source tarball":
(https://github.com/bitcoin/bitcoin/pull/33494#issuecomment-3382931665)
> I don't understand how this was ever intended to work. A depends source has a single hash.. that's pretty fundamental to the way it was designed. And a filename with a new hash is exactly the kind of thing it was intended to catch and disallow.
>
>
>
> The filename was changed upstream, and rightly so. Why did we not just adopt the new filename/hash and let that be the end of it?
>
That was the initial approach in https://github.com/bitcoin/bitcoin/commit/980e4987db0450cabb8761433001841f7e
...
(https://github.com/bitcoin/bitcoin/pull/33494#issuecomment-3382931665)
> I don't understand how this was ever intended to work. A depends source has a single hash.. that's pretty fundamental to the way it was designed. And a filename with a new hash is exactly the kind of thing it was intended to catch and disallow.
>
>
>
> The filename was changed upstream, and rightly so. Why did we not just adopt the new filename/hash and let that be the end of it?
>
That was the initial approach in https://github.com/bitcoin/bitcoin/commit/980e4987db0450cabb8761433001841f7e
...
⚠️ Crypt-iQ opened an issue: "CanDirectFetch degrades HB compact block relay if blocks are rare"
(https://github.com/bitcoin/bitcoin/issues/33578)
[`CanDirectFetch`](https://github.com/bitcoin/bitcoin/blob/b510893d00760083ac36948747aa6ebd84656192/src/net_processing.cpp#L1324) returns true when a node's local time is within 200 minutes of that node's tip time. When receiving a `CMPCTBLOCK` message from a peer, the node will [first check](https://github.com/bitcoin/bitcoin/blob/b510893d00760083ac36948747aa6ebd84656192/src/net_processing.cpp#L4428) if there is an in-flight block request for the block hash. If the block is not already requeste
...
(https://github.com/bitcoin/bitcoin/issues/33578)
[`CanDirectFetch`](https://github.com/bitcoin/bitcoin/blob/b510893d00760083ac36948747aa6ebd84656192/src/net_processing.cpp#L1324) returns true when a node's local time is within 200 minutes of that node's tip time. When receiving a `CMPCTBLOCK` message from a peer, the node will [first check](https://github.com/bitcoin/bitcoin/blob/b510893d00760083ac36948747aa6ebd84656192/src/net_processing.cpp#L4428) if there is an in-flight block request for the block hash. If the block is not already requeste
...