💬 maflcko commented on pull request "ci: remove `doc/release-notes.md` from lint-spelling.py":
(https://github.com/bitcoin/bitcoin/pull/33968#issuecomment-3607571201)
That could be fixed by rewording, or changing the spacing, maybe something like:
```diff
diff --git a/test/lint/test_runner/src/main.rs b/test/lint/test_runner/src/main.rs
index 5c65fcad89..39ae9dc815 100644
--- a/test/lint/test_runner/src/main.rs
+++ b/test/lint/test_runner/src/main.rs
@@ -742,7 +742,7 @@ fn run_all_python_linters() -> LintResult {
.success()
{
good = false;
- println!("^---- ⚠️ Failure generated from {entry_fn}");
...
(https://github.com/bitcoin/bitcoin/pull/33968#issuecomment-3607571201)
That could be fixed by rewording, or changing the spacing, maybe something like:
```diff
diff --git a/test/lint/test_runner/src/main.rs b/test/lint/test_runner/src/main.rs
index 5c65fcad89..39ae9dc815 100644
--- a/test/lint/test_runner/src/main.rs
+++ b/test/lint/test_runner/src/main.rs
@@ -742,7 +742,7 @@ fn run_all_python_linters() -> LintResult {
.success()
{
good = false;
- println!("^---- ⚠️ Failure generated from {entry_fn}");
...
💬 ryanofsky commented on issue "should concurrent IPC requests directed to the same thread cause a crash?":
(https://github.com/bitcoin/bitcoin/issues/33923#issuecomment-3607727965)
re: https://github.com/bitcoin/bitcoin/issues/33923#issuecomment-3566852691
> my main question is whether there are there protection mechanisms in place in case we try to create too many threads at once?
There aren't any currently, and making a lot of threads is probably the easiest way an IPC client could DoS the node.
It wouldn't be hard to create a limit though, if there is a use-case.
I do think creating 5 threads for each client that needs to receive templates sounds like overkill, and
...
(https://github.com/bitcoin/bitcoin/issues/33923#issuecomment-3607727965)
re: https://github.com/bitcoin/bitcoin/issues/33923#issuecomment-3566852691
> my main question is whether there are there protection mechanisms in place in case we try to create too many threads at once?
There aren't any currently, and making a lot of threads is probably the easiest way an IPC client could DoS the node.
It wouldn't be hard to create a limit though, if there is a use-case.
I do think creating 5 threads for each client that needs to receive templates sounds like overkill, and
...
👍 sedited approved a pull request: "validation: Improve warnings in case of chain corruption"
(https://github.com/bitcoin/bitcoin/pull/33553#pullrequestreview-3535952294)
ACK 4b4711369880369729893ba7baef11ba2a36cf4b
(https://github.com/bitcoin/bitcoin/pull/33553#pullrequestreview-3535952294)
ACK 4b4711369880369729893ba7baef11ba2a36cf4b
💬 maflcko commented on pull request "refactor: replace manual promise with SyncWithValidationInterfaceQueue":
(https://github.com/bitcoin/bitcoin/pull/33962#discussion_r2585968902)
@ANtutov are you still working on this?
(https://github.com/bitcoin/bitcoin/pull/33962#discussion_r2585968902)
@ANtutov are you still working on this?
👍 ryanofsky approved a pull request: "mining: getCoinbase() returns struct instead of raw tx"
(https://github.com/bitcoin/bitcoin/pull/33819#pullrequestreview-3536150722)
Code review ACK 60f74dd315811db427797a606f4b5611a8c59993. New comments and renaming both seem great, and it is nice to get rid of the extract function. PR feels much more straightforward now.
(https://github.com/bitcoin/bitcoin/pull/33819#pullrequestreview-3536150722)
Code review ACK 60f74dd315811db427797a606f4b5611a8c59993. New comments and renaming both seem great, and it is nice to get rid of the extract function. PR feels much more straightforward now.
💬 ryanofsky commented on pull request "mining: getCoinbase() returns struct instead of raw tx":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2586011866)
In commit "mining: add new getCoinbaseTx() returning a struct" (60f74dd315811db427797a606f4b5611a8c59993)
Would be nice to avoid the move and treat `pblock` and `coinbase_tx_template` local variables more consistently. Would suggest defining `CoinbaseTxTemplate& coinbase_tx_template{pblocktemplate->m_coinbase_tx_template}` above similar to `pblock`.
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2586011866)
In commit "mining: add new getCoinbaseTx() returning a struct" (60f74dd315811db427797a606f4b5611a8c59993)
Would be nice to avoid the move and treat `pblock` and `coinbase_tx_template` local variables more consistently. Would suggest defining `CoinbaseTxTemplate& coinbase_tx_template{pblocktemplate->m_coinbase_tx_template}` above similar to `pblock`.
💬 ryanofsky commented on pull request "mining: getCoinbase() returns struct instead of raw tx":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2585999526)
In commit "mining: add new getCoinbaseTx() returning a struct" (60f74dd315811db427797a606f4b5611a8c59993)
Maybe good to assert witness_index is between 0 and the vout size. Obviously it should be, but good to check array bounds since the index is coming from a different place.
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2585999526)
In commit "mining: add new getCoinbaseTx() returning a struct" (60f74dd315811db427797a606f4b5611a8c59993)
Maybe good to assert witness_index is between 0 and the vout size. Obviously it should be, but good to check array bounds since the index is coming from a different place.
💬 fanquake commented on pull request "guix: use GCC 14.3.0 over 13.3.0":
(https://github.com/bitcoin/bitcoin/pull/33775#issuecomment-3608054699)
@laanwj nice! I've re-integrated this into Guix (to build with `./contrib/guix/guix-build`), and minified the flags further, see this branch: https://github.com/fanquake/bitcoin/tree/repro_33775_minimal. With the top commit, the diff is still the register swapping:
```diff
--- a.txt
+++ b.txt
@@ -1,9 +1,9 @@
-test.cpp.obj.aarch64: file format pe-x86-64
+test.cpp.obj.x86_64: file format pe-x86-64
Disassembly of section .text:
0000000000000000 <IsBDBFile(fs::path cons
...
(https://github.com/bitcoin/bitcoin/pull/33775#issuecomment-3608054699)
@laanwj nice! I've re-integrated this into Guix (to build with `./contrib/guix/guix-build`), and minified the flags further, see this branch: https://github.com/fanquake/bitcoin/tree/repro_33775_minimal. With the top commit, the diff is still the register swapping:
```diff
--- a.txt
+++ b.txt
@@ -1,9 +1,9 @@
-test.cpp.obj.aarch64: file format pe-x86-64
+test.cpp.obj.x86_64: file format pe-x86-64
Disassembly of section .text:
0000000000000000 <IsBDBFile(fs::path cons
...
💬 hebasto commented on pull request "depends: Switch from multilib to platform-specific toolchains":
(https://github.com/bitcoin/bitcoin/pull/32162#issuecomment-3608103648)
My Guix build:
```
x86_64
9077d4894e4f4a5baa4763f65a61cc48a352312c59480cb9bb9395002a37e499 guix-build-7c2489086914/output/aarch64-linux-gnu/SHA256SUMS.part
b2efe0ce73341388f1f9c72b67448b9b864f80b48886b836beb975eae0d7ef86 guix-build-7c2489086914/output/aarch64-linux-gnu/bitcoin-7c2489086914-aarch64-linux-gnu-debug.tar.gz
ed13ca6437ea2604cc77357758bb578887cb6381d0511248bed941dc77a476d4 guix-build-7c2489086914/output/aarch64-linux-gnu/bitcoin-7c2489086914-aarch64-linux-gnu.tar.gz
6945b4b48
...
(https://github.com/bitcoin/bitcoin/pull/32162#issuecomment-3608103648)
My Guix build:
```
x86_64
9077d4894e4f4a5baa4763f65a61cc48a352312c59480cb9bb9395002a37e499 guix-build-7c2489086914/output/aarch64-linux-gnu/SHA256SUMS.part
b2efe0ce73341388f1f9c72b67448b9b864f80b48886b836beb975eae0d7ef86 guix-build-7c2489086914/output/aarch64-linux-gnu/bitcoin-7c2489086914-aarch64-linux-gnu-debug.tar.gz
ed13ca6437ea2604cc77357758bb578887cb6381d0511248bed941dc77a476d4 guix-build-7c2489086914/output/aarch64-linux-gnu/bitcoin-7c2489086914-aarch64-linux-gnu.tar.gz
6945b4b48
...
📝 instagibbs opened a pull request: "test: fix test_limit_enforcement_package"
(https://github.com/bitcoin/bitcoin/pull/34001)
The current test has a couple issues:
1) the parent_tx_good is regenerating the exact same transaction that is already in the cluster, so it's resulting in no replacements on submission
2) once fixed, the additional fee needs to be allocated to the parent transaction in the package, not the child. If the RBF fees are allocated to the child, this triggers the package RBF logic, which requires no in-mempool ancestors to be present.
Fix the bug and add a few assertions to protect against reg
...
(https://github.com/bitcoin/bitcoin/pull/34001)
The current test has a couple issues:
1) the parent_tx_good is regenerating the exact same transaction that is already in the cluster, so it's resulting in no replacements on submission
2) once fixed, the additional fee needs to be allocated to the parent transaction in the package, not the child. If the RBF fees are allocated to the child, this triggers the package RBF logic, which requires no in-mempool ancestors to be present.
Fix the bug and add a few assertions to protect against reg
...
💬 instagibbs commented on pull request "test: fix test_limit_enforcement_package":
(https://github.com/bitcoin/bitcoin/pull/34001#issuecomment-3608128047)
cc @sdaftuar @glozow
(https://github.com/bitcoin/bitcoin/pull/34001#issuecomment-3608128047)
cc @sdaftuar @glozow
🤔 w0xlt reviewed a pull request: "doc: improvements to doc/descriptors.md"
(https://github.com/bitcoin/bitcoin/pull/33986#pullrequestreview-3536298711)
ACK https://github.com/bitcoin/bitcoin/pull/33986/commits/fc33626f66e7f93f39989fbbabfbf00222762071
(https://github.com/bitcoin/bitcoin/pull/33986#pullrequestreview-3536298711)
ACK https://github.com/bitcoin/bitcoin/pull/33986/commits/fc33626f66e7f93f39989fbbabfbf00222762071
💬 rkrux commented on pull request "refactor: replace manual promise with SyncWithValidationInterfaceQueue":
(https://github.com/bitcoin/bitcoin/pull/33962#discussion_r2586156020)
Sorry for the late reply. I don't fully understand this comment though: https://github.com/bitcoin/bitcoin/pull/33962#discussion_r2577151438
I suggested to remove this variable because the `if` condition below could be managed by the `wait_callback` and `node.validation_signals` properties directly and specially setting the `callback_set` variable in an `if` block above that doesn't do anything else now seemed unnecessary to me.
(https://github.com/bitcoin/bitcoin/pull/33962#discussion_r2586156020)
Sorry for the late reply. I don't fully understand this comment though: https://github.com/bitcoin/bitcoin/pull/33962#discussion_r2577151438
I suggested to remove this variable because the `if` condition below could be managed by the `wait_callback` and `node.validation_signals` properties directly and specially setting the `callback_set` variable in an `if` block above that doesn't do anything else now seemed unnecessary to me.
👍 ryanofsky approved a pull request: "mining: fix -blockreservedweight shadows IPC option"
(https://github.com/bitcoin/bitcoin/pull/33965#pullrequestreview-3536257630)
Code review ACK 4d686142e079765cd31851481deb70659a9e9376. Just updated a comment since last review. Still looks good! Only minor comments below, not important
(https://github.com/bitcoin/bitcoin/pull/33965#pullrequestreview-3536257630)
Code review ACK 4d686142e079765cd31851481deb70659a9e9376. Just updated a comment since last review. Still looks good! Only minor comments below, not important
💬 ryanofsky commented on pull request "mining: fix -blockreservedweight shadows IPC option":
(https://github.com/bitcoin/bitcoin/pull/33965#discussion_r2586162219)
In commit "mining: fix -blockreservedweight shadows IPC option" (4d686142e079765cd31851481deb70659a9e9376)
Use MAX_BLOCK_WEIGHT constant?
(https://github.com/bitcoin/bitcoin/pull/33965#discussion_r2586162219)
In commit "mining: fix -blockreservedweight shadows IPC option" (4d686142e079765cd31851481deb70659a9e9376)
Use MAX_BLOCK_WEIGHT constant?
💬 ryanofsky commented on pull request "mining: fix -blockreservedweight shadows IPC option":
(https://github.com/bitcoin/bitcoin/pull/33965#discussion_r2586171036)
In commit "mining: fix -blockreservedweight shadows IPC option" (4d686142e079765cd31851481deb70659a9e9376)
Note: I guess if it did timeout you would get a confusing server error about trying to call a method on a null capability, so this error should be clearer
(https://github.com/bitcoin/bitcoin/pull/33965#discussion_r2586171036)
In commit "mining: fix -blockreservedweight shadows IPC option" (4d686142e079765cd31851481deb70659a9e9376)
Note: I guess if it did timeout you would get a confusing server error about trying to call a method on a null capability, so this error should be clearer
💬 ryanofsky commented on pull request "mining: fix -blockreservedweight shadows IPC option":
(https://github.com/bitcoin/bitcoin/pull/33965#discussion_r2586089447)
re: https://github.com/bitcoin/bitcoin/pull/33965#discussion_r2582425482
Yes that makes sense. I completely agree `MiningArgs` in #33966 should not use `std::optional` and that PR is correctly applying `DEFAULT_BLOCK_RESERVED_WEIGHT` in one place at the `ArgsMan` level, not the mining interface level.
I just don't think the implementation of this PR should be muddled in anticipation of #33966 before `MiningArgs` is introduced. In this PR, it makes sense to apply `DEFAULT_BLOCK_RESERVED_WEI
...
(https://github.com/bitcoin/bitcoin/pull/33965#discussion_r2586089447)
re: https://github.com/bitcoin/bitcoin/pull/33965#discussion_r2582425482
Yes that makes sense. I completely agree `MiningArgs` in #33966 should not use `std::optional` and that PR is correctly applying `DEFAULT_BLOCK_RESERVED_WEIGHT` in one place at the `ArgsMan` level, not the mining interface level.
I just don't think the implementation of this PR should be muddled in anticipation of #33966 before `MiningArgs` is introduced. In this PR, it makes sense to apply `DEFAULT_BLOCK_RESERVED_WEI
...
💬 ryanofsky commented on pull request "mining: fix -blockreservedweight shadows IPC option":
(https://github.com/bitcoin/bitcoin/pull/33965#discussion_r2586111251)
In commit "mining: fix -blockreservedweight shadows IPC option" (4d686142e079765cd31851481deb70659a9e9376)
Maybe update the comment to say this is being set to a high value to confirm that the -blockreservedweight option has no effect (IIUC). Current comment raises question of why the test is going out of its way to set an option that has no effect.
(https://github.com/bitcoin/bitcoin/pull/33965#discussion_r2586111251)
In commit "mining: fix -blockreservedweight shadows IPC option" (4d686142e079765cd31851481deb70659a9e9376)
Maybe update the comment to say this is being set to a high value to confirm that the -blockreservedweight option has no effect (IIUC). Current comment raises question of why the test is going out of its way to set an option that has no effect.
💬 fanquake commented on pull request "guix: use GCC 14.3.0 over 13.3.0":
(https://github.com/bitcoin/bitcoin/pull/33775#issuecomment-3608239216)
The non-determinism also goes away if you build with `_FORTIFY_SOURCE=2` instead of `_FORTIFY_SOURCE=3`. The difference there should be that the headers are using `__builtin_dynamic_object_size` in the `=3` case, instead of `__builtin_object_size`.
(https://github.com/bitcoin/bitcoin/pull/33775#issuecomment-3608239216)
The non-determinism also goes away if you build with `_FORTIFY_SOURCE=2` instead of `_FORTIFY_SOURCE=3`. The difference there should be that the headers are using `__builtin_dynamic_object_size` in the `=3` case, instead of `__builtin_object_size`.
💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2586259538)
Looks good, thanks!
https://github.com/bitcoin/bitcoin/compare/f07c765aa6bdba2511ceec56aa7f9755fa29a81e..d2f4bccbf70031b84b6af1a6c8480c4b3071bfd7
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2586259538)
Looks good, thanks!
https://github.com/bitcoin/bitcoin/compare/f07c765aa6bdba2511ceec56aa7f9755fa29a81e..d2f4bccbf70031b84b6af1a6c8480c4b3071bfd7