💬 fjahr commented on pull request "lint: Refactor lint checks to reuse exclusion logic; Support running individual lint checks in lint runner":
(https://github.com/bitcoin/bitcoin/pull/29744#issuecomment-2057047583)
> That comment says `so that they can use the already existing function` as the motivation. (This is the issue that was attempted to be solved and why the other pull request was created in the first place).
Right, and that original PR was merged so that part does not apply here anymore, still this PR is still open.
> Re-reading my comment, I don't get the impression that it said "using more Rust just for the sake of using more Rust in the project can be done".
Well, my impression is th
...
(https://github.com/bitcoin/bitcoin/pull/29744#issuecomment-2057047583)
> That comment says `so that they can use the already existing function` as the motivation. (This is the issue that was attempted to be solved and why the other pull request was created in the first place).
Right, and that original PR was merged so that part does not apply here anymore, still this PR is still open.
> Re-reading my comment, I don't get the impression that it said "using more Rust just for the sake of using more Rust in the project can be done".
Well, my impression is th
...
💬 maflcko commented on pull request "lint: Refactor lint checks to reuse exclusion logic; Support running individual lint checks in lint runner":
(https://github.com/bitcoin/bitcoin/pull/29744#issuecomment-2057069224)
> Right, and that original PR was merged so that part does not apply here anymore, still this PR is still open.
My understanding is that the original PR didn't actually fix the issue, so the problem remains. For example, `crypto/ctaes` is a subtree, but `test/lint/lint_ignore_dirs.py` does not list it. Also, `test/lint/lint-locale-dependence.py` and `test/lint/lint-python-utf8-encoding.py` don't use `test/lint/lint_ignore_dirs.py`.
Let me know if I am missing something. If the issue is ind
...
(https://github.com/bitcoin/bitcoin/pull/29744#issuecomment-2057069224)
> Right, and that original PR was merged so that part does not apply here anymore, still this PR is still open.
My understanding is that the original PR didn't actually fix the issue, so the problem remains. For example, `crypto/ctaes` is a subtree, but `test/lint/lint_ignore_dirs.py` does not list it. Also, `test/lint/lint-locale-dependence.py` and `test/lint/lint-python-utf8-encoding.py` don't use `test/lint/lint_ignore_dirs.py`.
Let me know if I am missing something. If the issue is ind
...
💬 fjahr commented on pull request "lint: Refactor lint checks to reuse exclusion logic; Support running individual lint checks in lint runner":
(https://github.com/bitcoin/bitcoin/pull/29744#issuecomment-2057113596)
> For example, crypto/ctaes is a subtree, but test/lint/lint_ignore_dirs.py does not list it. Also, test/lint/lint-locale-dependence.py and test/lint/lint-python-utf8-encoding.py don't use test/lint/lint_ignore_dirs.py.
Right, these are behavior changes that I did not see as part of the motivation of the original PR, as the title says it was a refactor. So I don't think this is something that was missed, it was just not the goal. It should be made more clear that this is the main motivation
...
(https://github.com/bitcoin/bitcoin/pull/29744#issuecomment-2057113596)
> For example, crypto/ctaes is a subtree, but test/lint/lint_ignore_dirs.py does not list it. Also, test/lint/lint-locale-dependence.py and test/lint/lint-python-utf8-encoding.py don't use test/lint/lint_ignore_dirs.py.
Right, these are behavior changes that I did not see as part of the motivation of the original PR, as the title says it was a refactor. So I don't think this is something that was missed, it was just not the goal. It should be made more clear that this is the main motivation
...
💬 fanquake commented on pull request "depends: build miniupnpc with CMake":
(https://github.com/bitcoin/bitcoin/pull/29707#issuecomment-2057119905)
Pushed for builds now that the mirrors are back up.
(https://github.com/bitcoin/bitcoin/pull/29707#issuecomment-2057119905)
Pushed for builds now that the mirrors are back up.
💬 fanquake commented on pull request "depends: build expat with CMake":
(https://github.com/bitcoin/bitcoin/pull/29878#issuecomment-2057124280)
Looks like there could be issues with expat, CMake and 32-bit builds:
* https://cirrus-ci.com/task/6695396838211584
* https://cirrus-ci.com/task/5850971908079616
(https://github.com/bitcoin/bitcoin/pull/29878#issuecomment-2057124280)
Looks like there could be issues with expat, CMake and 32-bit builds:
* https://cirrus-ci.com/task/6695396838211584
* https://cirrus-ci.com/task/5850971908079616
💬 achow101 commented on issue "IBD performance regression in 27.0rc1 on Windows":
(https://github.com/bitcoin/bitcoin/issues/29785#issuecomment-2057136484)
> Did someone with Windows try to enable the benchmarks in guix and compile them, and run them?
Yes, I did that. I could not find any benchmark that had a significant difference.
(https://github.com/bitcoin/bitcoin/issues/29785#issuecomment-2057136484)
> Did someone with Windows try to enable the benchmarks in guix and compile them, and run them?
Yes, I did that. I could not find any benchmark that had a significant difference.
💬 fjahr commented on pull request "Safegcd-based modular inverses in MuHash3072":
(https://github.com/bitcoin/bitcoin/pull/21590#issuecomment-2057150785)
@sipa The description still lists "Add more tests" as an open todo. Did you still want to address this before this could be merged? I think we have ok-ish coverage of MuHash but if you think something should be added could you tell us what you had in mind? Are there maybe test vectors we can port over?
(https://github.com/bitcoin/bitcoin/pull/21590#issuecomment-2057150785)
@sipa The description still lists "Add more tests" as an open todo. Did you still want to address this before this could be merged? I think we have ok-ish coverage of MuHash but if you think something should be added could you tell us what you had in mind? Are there maybe test vectors we can port over?
💬 sipa commented on pull request "Safegcd-based modular inverses in MuHash3072":
(https://github.com/bitcoin/bitcoin/pull/21590#issuecomment-2057153942)
@fjahr I've dropped the TODO. Feel free to contribute tests of course if you feel that's helpful.
(https://github.com/bitcoin/bitcoin/pull/21590#issuecomment-2057153942)
@fjahr I've dropped the TODO. Feel free to contribute tests of course if you feel that's helpful.
💬 instagibbs commented on pull request "fuzz: explicitly cap the vsize of RBFs for diagram checks":
(https://github.com/bitcoin/bitcoin/pull/29879#discussion_r1566001308)
this is hopefully a conservative over-estimate since clusters should only be 101kvB total each
(https://github.com/bitcoin/bitcoin/pull/29879#discussion_r1566001308)
this is hopefully a conservative over-estimate since clusters should only be 101kvB total each
💬 instagibbs 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_r1565931380)
is this strictly necessary since the mempool is empty?
fine to me to add as belt and suspenders either way
(https://github.com/bitcoin/bitcoin/pull/29827#discussion_r1565931380)
is this strictly necessary since the mempool is empty?
fine to me to add as belt and suspenders either way
👍 instagibbs approved a pull request: "test: p2p: add test for rejected tx request logic (`m_recent_rejects` filter)"
(https://github.com/bitcoin/bitcoin/pull/29827#pullrequestreview-2001340821)
ACK 01b17b62cbeb168629d17eb2a54d547628e16759
(https://github.com/bitcoin/bitcoin/pull/29827#pullrequestreview-2001340821)
ACK 01b17b62cbeb168629d17eb2a54d547628e16759
💬 instagibbs 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_r1565964264)
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?
(https://github.com/bitcoin/bitcoin/pull/29827#discussion_r1565964264)
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?
💬 achow101 commented on pull request "doc: update release-process.md":
(https://github.com/bitcoin/bitcoin/pull/29645#discussion_r1566005991)
I think we've decided to start uploading all build artifacts, including the debug files, so this sentence and section below can be removed.
(https://github.com/bitcoin/bitcoin/pull/29645#discussion_r1566005991)
I think we've decided to start uploading all build artifacts, including the debug files, so this sentence and section below can be removed.
💬 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:
...