💬 achow101 commented on pull request "doc: update release-process.md":
(https://github.com/bitcoin/bitcoin/pull/29645#discussion_r1566010280)
If we're going to mention torrent generation, maybe also mention the OpenTimestamps file too? Both are generated automatically on the server now.
(https://github.com/bitcoin/bitcoin/pull/29645#discussion_r1566010280)
If we're going to mention torrent generation, maybe also mention the OpenTimestamps file too? Both are generated automatically on the server now.
💬 maflcko commented on issue "IBD performance regression in 27.0rc1 on Windows":
(https://github.com/bitcoin/bitcoin/issues/29785#issuecomment-2057185867)
Thanks for confirming. If the benchmarks can not show a difference for someone who could reproduce, bisecting guix builds may be the only option left to debug this, but that will probably take some time.
(https://github.com/bitcoin/bitcoin/issues/29785#issuecomment-2057185867)
Thanks for confirming. If the benchmarks can not show a difference for someone who could reproduce, bisecting guix builds may be the only option left to debug this, but that will probably take some time.
💬 achow101 commented on pull request "[27.x] More backports and finalize":
(https://github.com/bitcoin/bitcoin/pull/29780#issuecomment-2057187851)
ACK 910e3e8728b258a38d38f8f9ddf6b23406e8d5ce
(https://github.com/bitcoin/bitcoin/pull/29780#issuecomment-2057187851)
ACK 910e3e8728b258a38d38f8f9ddf6b23406e8d5ce
💬 sr-gi commented on pull request "Simplify network-adjusted time warning logic":
(https://github.com/bitcoin/bitcoin/pull/29623#discussion_r1566015813)
I don't think this was covered (or it may have been regressed)
(https://github.com/bitcoin/bitcoin/pull/29623#discussion_r1566015813)
I don't think this was covered (or it may have been regressed)
💬 sr-gi commented on pull request "Simplify network-adjusted time warning logic":
(https://github.com/bitcoin/bitcoin/pull/29623#issuecomment-2057192875)
Re-ACK [c6be144](https://github.com/bitcoin/bitcoin/pull/29623/commits/c6be144c4b774a03a8bcab5a165768cf81e9b06b)
(https://github.com/bitcoin/bitcoin/pull/29623#issuecomment-2057192875)
Re-ACK [c6be144](https://github.com/bitcoin/bitcoin/pull/29623/commits/c6be144c4b774a03a8bcab5a165768cf81e9b06b)
👍 alfonsoromanz approved a pull request: "test: remove duplicated ban test"
(https://github.com/bitcoin/bitcoin/pull/29688#pullrequestreview-2001500817)
ACK e30e8625bbc42045b8b757a8d7e80c20cc61cebf
(https://github.com/bitcoin/bitcoin/pull/29688#pullrequestreview-2001500817)
ACK e30e8625bbc42045b8b757a8d7e80c20cc61cebf
💬 laanwj commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#issuecomment-2057202451)
i tried some old wallets, and there was a problem with parsing the headers of overflow pages (which have btree level 0), the following makes the check pass and adds a further check that overflow records correspond to overflow pages:
```patch
diff --git a/src/wallet/migrate.cpp b/src/wallet/migrate.cpp
index be54b58f67c9bc961f8df306fe4bc0bf810b41f3..1c4ce1c1ed8d44221626613df2cd0d4712fc5c85 100644
--- a/src/wallet/migrate.cpp
+++ b/src/wallet/migrate.cpp
@@ -361,7 +361,7 @@ public:
...
(https://github.com/bitcoin/bitcoin/pull/26606#issuecomment-2057202451)
i tried some old wallets, and there was a problem with parsing the headers of overflow pages (which have btree level 0), the following makes the check pass and adds a further check that overflow records correspond to overflow pages:
```patch
diff --git a/src/wallet/migrate.cpp b/src/wallet/migrate.cpp
index be54b58f67c9bc961f8df306fe4bc0bf810b41f3..1c4ce1c1ed8d44221626613df2cd0d4712fc5c85 100644
--- a/src/wallet/migrate.cpp
+++ b/src/wallet/migrate.cpp
@@ -361,7 +361,7 @@ public:
...
💬 sipa commented on pull request "fuzz: explicitly cap the vsize of RBFs for diagram checks":
(https://github.com/bitcoin/bitcoin/pull/29879#issuecomment-2057203936)
I'm not convinced about this approach. A nice thing about fuzz test is that they can be written to exercise scenarios which go beyond what matters in practice, as more extreme values may trigger bugs more easily (and those bugs may affect realistic cases too, but be harder to trigger there).
More specifically, in this case it seems the issue is the code under test not working if the sum of sizes exceeds 2^31. The standard transaction size shouldn't matter for this code, so I'd prefer to restr
...
(https://github.com/bitcoin/bitcoin/pull/29879#issuecomment-2057203936)
I'm not convinced about this approach. A nice thing about fuzz test is that they can be written to exercise scenarios which go beyond what matters in practice, as more extreme values may trigger bugs more easily (and those bugs may affect realistic cases too, but be harder to trigger there).
More specifically, in this case it seems the issue is the code under test not working if the sum of sizes exceeds 2^31. The standard transaction size shouldn't matter for this code, so I'd prefer to restr
...
💬 brunoerg commented on pull request "test: check disconnection when sending sendaddrv2 after verack":
(https://github.com/bitcoin/bitcoin/pull/29699#discussion_r1566030227)
Yes :)
(https://github.com/bitcoin/bitcoin/pull/29699#discussion_r1566030227)
Yes :)
💬 theStack commented on pull request "test: p2p: add test for rejected tx request logic (`m_recent_rejects` filter)":
(https://github.com/bitcoin/bitcoin/pull/29827#discussion_r1566039065)
> is this strictly necessary since the mempool is empty?
Good point. Without that change the test fails, but not for the reason I thought it does. I think the problem is that MiniWallet is not aware of restarts between nodes and still holds mempool UTXOs that it tries to spend. Will force-push in a bit with a fix that does a `self.wallet.rescan_utxos()` after restarts, which should tackle the problem as the root.
(https://github.com/bitcoin/bitcoin/pull/29827#discussion_r1566039065)
> is this strictly necessary since the mempool is empty?
Good point. Without that change the test fails, but not for the reason I thought it does. I think the problem is that MiniWallet is not aware of restarts between nodes and still holds mempool UTXOs that it tries to spend. Will force-push in a bit with a fix that does a `self.wallet.rescan_utxos()` after restarts, which should tackle the problem as the root.
💬 stickies-v commented on pull request "Simplify network-adjusted time warning logic":
(https://github.com/bitcoin/bitcoin/pull/29623#discussion_r1566042673)
Sorry, I think it must have regressed with [this update in the next line](https://github.com/bitcoin/bitcoin/compare/2ef71c73582be554e565ada3f8a6ca77c2cd79f1..5362db10ab6d1d6697807c8f8494a2fc5bf41ff8#diff-6875de769e90cec84d2e8a9c1b962cdbcda44d870d42e4215827e599e11e90e3R3687) - will fix if I have to retouch.
(https://github.com/bitcoin/bitcoin/pull/29623#discussion_r1566042673)
Sorry, I think it must have regressed with [this update in the next line](https://github.com/bitcoin/bitcoin/compare/2ef71c73582be554e565ada3f8a6ca77c2cd79f1..5362db10ab6d1d6697807c8f8494a2fc5bf41ff8#diff-6875de769e90cec84d2e8a9c1b962cdbcda44d870d42e4215827e599e11e90e3R3687) - will fix if I have to retouch.
💬 alfonsoromanz commented on pull request "Feature: Use different datadirs for different signets":
(https://github.com/bitcoin/bitcoin/pull/29838#issuecomment-2057231800)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/29838#issuecomment-2057231800)
Concept ACK
💬 instagibbs commented on pull request "fuzz: explicitly cap the vsize of RBFs for diagram checks":
(https://github.com/bitcoin/bitcoin/pull/29879#issuecomment-2057240637)
> there is no strict need to tie it to standard sizes.
Pretty sure we can overflow if we allow ~1MvB transactions, as `10,000 * 1,000,000 > 2^31`
Made a more direct limiting attempt. I will squash if it is preferred
(https://github.com/bitcoin/bitcoin/pull/29879#issuecomment-2057240637)
> there is no strict need to tie it to standard sizes.
Pretty sure we can overflow if we allow ~1MvB transactions, as `10,000 * 1,000,000 > 2^31`
Made a more direct limiting attempt. I will squash if it is preferred
🤔 BrandonOdiwuor reviewed a pull request: "Add Signet launch shortcut for Windows"
(https://github.com/bitcoin/bitcoin/pull/26334#pullrequestreview-2001547908)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/26334#pullrequestreview-2001547908)
Concept ACK
📝 fanquake opened a pull request: "depends: build FreeType with CMake"
(https://github.com/bitcoin/bitcoin/pull/29880)
Bumps FreeType from [`2.11.0`](https://sourceforge.net/projects/freetype/files/freetype2/2.11.0/) to [`2.13.2`](https://sourceforge.net/projects/freetype/files/freetype2/2.13.2/) (currently untested/reviewed).
Switches Freetype to be built with CMake.
(https://github.com/bitcoin/bitcoin/pull/29880)
Bumps FreeType from [`2.11.0`](https://sourceforge.net/projects/freetype/files/freetype2/2.11.0/) to [`2.13.2`](https://sourceforge.net/projects/freetype/files/freetype2/2.13.2/) (currently untested/reviewed).
Switches Freetype to be built with CMake.
🚀 fanquake merged a pull request: "[27.x] More backports and finalize"
(https://github.com/bitcoin/bitcoin/pull/29780)
(https://github.com/bitcoin/bitcoin/pull/29780)
💬 theStack commented on pull request "test: p2p: add test for rejected tx request logic (`m_recent_rejects` filter)":
(https://github.com/bitcoin/bitcoin/pull/29827#discussion_r1566089752)
> I'll punt to future work(TM) but I was wondering if we could just generate an "ephemeral" miniwallet inside `fill_mempool` if the resulting utxos from filling the mempool aren't necessary for the rest of the test?
Hmm for that purpose I think it would be nice if different MiniWallet instances create different output scripts, to avoid confusion. Currently, MiniWallet instances for the same output type (e.g. the default `ADDRESS_OP_TRUE`) always result in the same UTXO pool (at least, after t
...
(https://github.com/bitcoin/bitcoin/pull/29827#discussion_r1566089752)
> I'll punt to future work(TM) but I was wondering if we could just generate an "ephemeral" miniwallet inside `fill_mempool` if the resulting utxos from filling the mempool aren't necessary for the rest of the test?
Hmm for that purpose I think it would be nice if different MiniWallet instances create different output scripts, to avoid confusion. Currently, MiniWallet instances for the same output type (e.g. the default `ADDRESS_OP_TRUE`) always result in the same UTXO pool (at least, after t
...
💬 stickies-v commented on pull request "Simplify network-adjusted time warning logic":
(https://github.com/bitcoin/bitcoin/pull/29623#discussion_r1566121275)
We `#include <util/time.h>` which has `using namespace std::chrono_literals;` which is why it compiles. I thought we would prefer (IWYU-style) to be explicit about used namespaces, but upon further investigation the purpose of #20602 seems to have been to _avoid_ that, so I'll remove this if I re-touch.
(https://github.com/bitcoin/bitcoin/pull/29623#discussion_r1566121275)
We `#include <util/time.h>` which has `using namespace std::chrono_literals;` which is why it compiles. I thought we would prefer (IWYU-style) to be explicit about used namespaces, but upon further investigation the purpose of #20602 seems to have been to _avoid_ that, so I'll remove this if I re-touch.
💬 pinheadmz commented on issue "./bitcoin.conf file should not cause confusion with ./datadir/bitcoin.conf":
(https://github.com/bitcoin/bitcoin/issues/29139#issuecomment-2057375048)
> IMO folks looking for an example conf would also be more likely to look for and find it in `share/`.
agreed. im testing your commit in a guix build locally, will ack that PR
(https://github.com/bitcoin/bitcoin/issues/29139#issuecomment-2057375048)
> IMO folks looking for an example conf would also be more likely to look for and find it in `share/`.
agreed. im testing your commit in a guix build locally, will ack that PR
💬 stickies-v commented on pull request "rpc: return warnings as an array instead of just a single one":
(https://github.com/bitcoin/bitcoin/pull/29845#discussion_r1566154090)
I generally tend to prefer to doxygen-ify documentation, but in this case I'm not sure the extra line adds anything over the return type and function name, so then is it just unnecessary boilerplate that makes the documentation less instead of more clear? No strong view either way, so will leave this open.
(https://github.com/bitcoin/bitcoin/pull/29845#discussion_r1566154090)
I generally tend to prefer to doxygen-ify documentation, but in this case I'm not sure the extra line adds anything over the return type and function name, so then is it just unnecessary boilerplate that makes the documentation less instead of more clear? No strong view either way, so will leave this open.