💬 murchandamus commented on pull request "Reduce minrelaytxfee to 100 sats/kvB":
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3110475043)
Maybe the intent of my comment wasn’t clear. We are seeing that blocks including lower feerate transactions take significantly longer to get propagated, and the way I’m reading the sentiment here, a lot of people do not share my concern over a potential DoS protection reduction. In so far, it seems that there is at least interest in this PR being further developed. My suggestion is that if someone were to address the review feedback here, they should probably lower all three settings, not just t
...
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3110475043)
Maybe the intent of my comment wasn’t clear. We are seeing that blocks including lower feerate transactions take significantly longer to get propagated, and the way I’m reading the sentiment here, a lot of people do not share my concern over a potential DoS protection reduction. In so far, it seems that there is at least interest in this PR being further developed. My suggestion is that if someone were to address the review feedback here, they should probably lower all three settings, not just t
...
💬 achow101 commented on pull request "doc: update headers and TOC in `developer-notes.md`":
(https://github.com/bitcoin/bitcoin/pull/33040#issuecomment-3110481836)
I would prefer to drop the table of contents entirely.
I pretty much only ever read `.md` files on GitHub or with `mdcat`. On GitHub, the table of contents will be automatically generated, and with `mdcat`, the links don't work anyways.
(https://github.com/bitcoin/bitcoin/pull/33040#issuecomment-3110481836)
I would prefer to drop the table of contents entirely.
I pretty much only ever read `.md` files on GitHub or with `mdcat`. On GitHub, the table of contents will be automatically generated, and with `mdcat`, the links don't work anyways.
🤔 stickies-v reviewed a pull request: "log: rate limiting followups"
(https://github.com/bitcoin/bitcoin/pull/33011#pullrequestreview-3046739749)
code lgtm 8aa2726f1cfbc1e4267a796cb000c4eeae5910f5 - but would like to see if we can make improvements on the `m_limiter` lifetime guarantees, and on the `logging_filesize_rate_limit` tests. Both are not essential, but I think it could make sense to include them here.
(https://github.com/bitcoin/bitcoin/pull/33011#pullrequestreview-3046739749)
code lgtm 8aa2726f1cfbc1e4267a796cb000c4eeae5910f5 - but would like to see if we can make improvements on the `m_limiter` lifetime guarantees, and on the `logging_filesize_rate_limit` tests. Both are not essential, but I think it could make sense to include them here.
💬 stickies-v commented on pull request "log: rate limiting followups":
(https://github.com/bitcoin/bitcoin/pull/33011#discussion_r2225128347)
nit on 35585c54b57bbb05bfc4de63880c1767931e3696: more consistent parameterise both:
<details>
<summary>git diff on d0b3a80b7f</summary>
```diff
diff --git a/src/init.cpp b/src/init.cpp
index 63244c802e..863b2e088e 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -1382,7 +1382,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
LogInstance().SetRateLimiting(std::make_unique<BCLog::LogRateLimiter>(
[&scheduler](auto func, auto window) {
...
(https://github.com/bitcoin/bitcoin/pull/33011#discussion_r2225128347)
nit on 35585c54b57bbb05bfc4de63880c1767931e3696: more consistent parameterise both:
<details>
<summary>git diff on d0b3a80b7f</summary>
```diff
diff --git a/src/init.cpp b/src/init.cpp
index 63244c802e..863b2e088e 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -1382,7 +1382,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
LogInstance().SetRateLimiting(std::make_unique<BCLog::LogRateLimiter>(
[&scheduler](auto func, auto window) {
...
💬 stickies-v commented on pull request "log: rate limiting followups":
(https://github.com/bitcoin/bitcoin/pull/33011#discussion_r2225171353)
nit: using descriptive names might make more sense, e.g. `INFO_1`, `INFO_2`, `DEBUG`, `INFO_NOLIMIT`
(https://github.com/bitcoin/bitcoin/pull/33011#discussion_r2225171353)
nit: using descriptive names might make more sense, e.g. `INFO_1`, `INFO_2`, `DEBUG`, `INFO_NOLIMIT`
💬 stickies-v commented on pull request "log: rate limiting followups":
(https://github.com/bitcoin/bitcoin/pull/33011#discussion_r2225484458)
Note: this is already handled by the `LogSetup` fixture, so I think we can remove this logic here.
On that note: I think `LogSetup` could benefit from explicitly setting `LogInstance().SetRateLimiting(nullptr);` in both its constructor and destructor?
(https://github.com/bitcoin/bitcoin/pull/33011#discussion_r2225484458)
Note: this is already handled by the `LogSetup` fixture, so I think we can remove this logic here.
On that note: I think `LogSetup` could benefit from explicitly setting `LogInstance().SetRateLimiting(nullptr);` in both its constructor and destructor?
💬 stickies-v commented on pull request "log: rate limiting followups":
(https://github.com/bitcoin/bitcoin/pull/33011#discussion_r2225184386)
nit: I find setting `line_length{1024}` and documenting the subtraction more straightforward:
<details>
<summary>git diff on d0b3a80b7f</summary>
```diff
diff --git a/src/test/logging_tests.cpp b/src/test/logging_tests.cpp
index 81c0bacba0..245940f284 100644
--- a/src/test/logging_tests.cpp
+++ b/src/test/logging_tests.cpp
@@ -414,11 +414,10 @@ BOOST_FIXTURE_TEST_CASE(logging_filesize_rate_limit, LogSetup)
bool prev_log_threadnames{LogInstance().m_log_threadnames};
LogIns
...
(https://github.com/bitcoin/bitcoin/pull/33011#discussion_r2225184386)
nit: I find setting `line_length{1024}` and documenting the subtraction more straightforward:
<details>
<summary>git diff on d0b3a80b7f</summary>
```diff
diff --git a/src/test/logging_tests.cpp b/src/test/logging_tests.cpp
index 81c0bacba0..245940f284 100644
--- a/src/test/logging_tests.cpp
+++ b/src/test/logging_tests.cpp
@@ -414,11 +414,10 @@ BOOST_FIXTURE_TEST_CASE(logging_filesize_rate_limit, LogSetup)
bool prev_log_threadnames{LogInstance().m_log_threadnames};
LogIns
...
💬 stickies-v commented on pull request "log: rate limiting followups":
(https://github.com/bitcoin/bitcoin/pull/33011#discussion_r2226805223)
The test has definitely improved in both functionality and readability, but I find it's still quite clunky (e.g. lots of repeated code) and unspecific (e.g. using any increase in filesize as measurement, not accurately checking the (non) existence of certain log statements most of the time), so I dove into a little rabbit hole today to see what my ideal test would look like, and I think it'd be close to something like this: https://github.com/stickies-v/bitcoin/commit/8aa2726f1cfbc1e4267a796cb00
...
(https://github.com/bitcoin/bitcoin/pull/33011#discussion_r2226805223)
The test has definitely improved in both functionality and readability, but I find it's still quite clunky (e.g. lots of repeated code) and unspecific (e.g. using any increase in filesize as measurement, not accurately checking the (non) existence of certain log statements most of the time), so I dove into a little rabbit hole today to see what my ideal test would look like, and I think it'd be close to something like this: https://github.com/stickies-v/bitcoin/commit/8aa2726f1cfbc1e4267a796cb00
...
💬 stickies-v commented on pull request "log: rate limiting followups":
(https://github.com/bitcoin/bitcoin/pull/33011#discussion_r2225156552)
nit: `auto` can sometimes make things less readable, but I think in all of these changes that's not the case. Repeating types (e.g. `std::source_location source_loc_1{std::source_location::current()}` is just overly verbose imo), and for the others the name/literal is already explanatory. For `sched_func`, I think the logic is now drowned in rather unimportant type definitions. I think this addresses [this comment](https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2188256330), but I pers
...
(https://github.com/bitcoin/bitcoin/pull/33011#discussion_r2225156552)
nit: `auto` can sometimes make things less readable, but I think in all of these changes that's not the case. Repeating types (e.g. `std::source_location source_loc_1{std::source_location::current()}` is just overly verbose imo), and for the others the name/literal is already explanatory. For `sched_func`, I think the logic is now drowned in rather unimportant type definitions. I think this addresses [this comment](https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2188256330), but I pers
...
💬 stickies-v commented on pull request "log: rate limiting followups":
(https://github.com/bitcoin/bitcoin/pull/33011#discussion_r2226684372)
There are some lifetime concerns wrt `scheduler_func`, that are okay in our current codebase, but make the contract perhaps more fragile than it needs to be. Specifically: `Logger` has a `std::unique_ptr<LogRateLimiter> m_limiter` member. The node's scheduler holds [a reference](https://github.com/bitcoin/bitcoin/blob/eb137184482cea8a2a7cdfcd3c73ec6ae5fdd0d6/src/logging.cpp#L379) to the limiter. This means that the limiter may not be destroyed before the scheduler is stopped. In our current code
...
(https://github.com/bitcoin/bitcoin/pull/33011#discussion_r2226684372)
There are some lifetime concerns wrt `scheduler_func`, that are okay in our current codebase, but make the contract perhaps more fragile than it needs to be. Specifically: `Logger` has a `std::unique_ptr<LogRateLimiter> m_limiter` member. The node's scheduler holds [a reference](https://github.com/bitcoin/bitcoin/blob/eb137184482cea8a2a7cdfcd3c73ec6ae5fdd0d6/src/logging.cpp#L379) to the limiter. This means that the limiter may not be destroyed before the scheduler is stopped. In our current code
...
💬 stickies-v commented on pull request "log: rate limiting followups":
(https://github.com/bitcoin/bitcoin/pull/33011#discussion_r2225276727)
nit: use variable
```suggestion
MockForwardAndSync(scheduler, time_window);
```
(https://github.com/bitcoin/bitcoin/pull/33011#discussion_r2225276727)
nit: use variable
```suggestion
MockForwardAndSync(scheduler, time_window);
```
💬 l0rinc commented on pull request "doc: update headers and TOC in `developer-notes.md`":
(https://github.com/bitcoin/bitcoin/pull/33040#issuecomment-3110641027)
I'm fine with that option as well, though it's not the only doc with a TOC:
* https://github.com/bitcoin/bitcoin/blob/master/doc/productivity.md *
* https://github.com/bitcoin/bitcoin/blob/master/doc/design/multiprocess.md *
* https://github.com/bitcoin/bitcoin/blob/master/doc/files.md
The first two are also missing headers from the TOC - the last one seems up-to-date.
So I agree - we either update them all or delete the TOC in all 3 docs.
(https://github.com/bitcoin/bitcoin/pull/33040#issuecomment-3110641027)
I'm fine with that option as well, though it's not the only doc with a TOC:
* https://github.com/bitcoin/bitcoin/blob/master/doc/productivity.md *
* https://github.com/bitcoin/bitcoin/blob/master/doc/design/multiprocess.md *
* https://github.com/bitcoin/bitcoin/blob/master/doc/files.md
The first two are also missing headers from the TOC - the last one seems up-to-date.
So I agree - we either update them all or delete the TOC in all 3 docs.
🤔 jonatack reviewed a pull request: "Introduce SockMan ("lite"): low-level socket handling for HTTP"
(https://github.com/bitcoin/bitcoin/pull/32747#pullrequestreview-3049290299)
Began reviewing, then realized that I was reviewing the same code twice with the two "modernize" commits following "original" commits.
Propose to either (1) make clear in the commit message where the code is being pulled from, and that it will be re-styled afterward, or (2) preferably squash the modernization commits into the previous ones, thereby easing life a bit for your reviewers :)
(https://github.com/bitcoin/bitcoin/pull/32747#pullrequestreview-3049290299)
Began reviewing, then realized that I was reviewing the same code twice with the two "modernize" commits following "original" commits.
Propose to either (1) make clear in the commit message where the code is being pulled from, and that it will be re-styled afterward, or (2) preferably squash the modernization commits into the previous ones, thereby easing life a bit for your reviewers :)
💬 jonatack commented on pull request "Introduce SockMan ("lite"): low-level socket handling for HTTP":
(https://github.com/bitcoin/bitcoin/pull/32747#discussion_r2226846994)
535daaf15fd754335116d17833a45261cdff4e93 nit, sort
(https://github.com/bitcoin/bitcoin/pull/32747#discussion_r2226846994)
535daaf15fd754335116d17833a45261cdff4e93 nit, sort
💬 jonatack commented on pull request "Introduce SockMan ("lite"): low-level socket handling for HTTP":
(https://github.com/bitcoin/bitcoin/pull/32747#discussion_r2226848616)
535daaf15fd754335116d17833a45261cdff4e93
```suggestion
// Copyright (c) 2025-present The Bitcoin Core developers
```
(https://github.com/bitcoin/bitcoin/pull/32747#discussion_r2226848616)
535daaf15fd754335116d17833a45261cdff4e93
```suggestion
// Copyright (c) 2025-present The Bitcoin Core developers
```
🤔 w0xlt reviewed a pull request: "wallet, sqlite: Encapsulate SQLite statements in a RAII class"
(https://github.com/bitcoin/bitcoin/pull/33033#pullrequestreview-3049382506)
Looks good to me
ACK https://github.com/bitcoin/bitcoin/pull/33033/commits/b2c824a3ce0877f2cb7cbed1731088ed8ba6171a
(https://github.com/bitcoin/bitcoin/pull/33033#pullrequestreview-3049382506)
Looks good to me
ACK https://github.com/bitcoin/bitcoin/pull/33033/commits/b2c824a3ce0877f2cb7cbed1731088ed8ba6171a
💬 achow101 commented on pull request "Split `CWallet::Create()` into `CreateNew` and `LoadExisting`":
(https://github.com/bitcoin/bitcoin/pull/32636#discussion_r2226917021)
In f26bf5fd24ffed501dfdb3528c56cfaee0203a72 "scripted-diff: refactor: rename CWallet::LoadWallet"
I think we could actually drop the `PopulateWalletFromDB` here and in other test setups that use `MockableDatabase` since there's nothing in the databases anyways to load. These calls should be no-ops.
(https://github.com/bitcoin/bitcoin/pull/32636#discussion_r2226917021)
In f26bf5fd24ffed501dfdb3528c56cfaee0203a72 "scripted-diff: refactor: rename CWallet::LoadWallet"
I think we could actually drop the `PopulateWalletFromDB` here and in other test setups that use `MockableDatabase` since there's nothing in the databases anyways to load. These calls should be no-ops.
💬 tnndbtc commented on issue "intermittent issue in rpc_signer.py (enumeratesigners timeout) under GLIBCXX debug mode":
(https://github.com/bitcoin/bitcoin/issues/32524#issuecomment-3111442116)
Since it happened again, should we consider adding more error/exception handlings on `subprocess_close`? Currently we assume it's always successful, but it may return -1 as error code.
(https://github.com/bitcoin/bitcoin/issues/32524#issuecomment-3111442116)
Since it happened again, should we consider adding more error/exception handlings on `subprocess_close`? Currently we assume it's always successful, but it may return -1 as error code.
🤔 l0rinc requested changes to a pull request: "refactor: Header sync optimisations & simplifications"
(https://github.com/bitcoin/bitcoin/pull/32740#pullrequestreview-3045078110)
Concept ACK, I'm not sure about the first change but the rest make sense.
reviewed f4b33a14f27bb3ee734e8d4eb9c2b2ccaa5df52a, left some comments for most commits, my main concern is the commit where we're reusing the headers vector, making the code significantly more contrived.
It would also be great if we could extract the common parts of this and @hodlinator's change and merge that (or his change) before we do this.
(https://github.com/bitcoin/bitcoin/pull/32740#pullrequestreview-3045078110)
Concept ACK, I'm not sure about the first change but the rest make sense.
reviewed f4b33a14f27bb3ee734e8d4eb9c2b2ccaa5df52a, left some comments for most commits, my main concern is the commit where we're reusing the headers vector, making the code significantly more contrived.
It would also be great if we could extract the common parts of this and @hodlinator's change and merge that (or his change) before we do this.
💬 l0rinc commented on pull request "refactor: Header sync optimisations & simplifications":
(https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2226896908)
It was already hard to follow this stateful test, now it's even more so - could we maybe cherry-pick @hodlinator's cleanup commit before of this change?
(https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2226896908)
It was already hard to follow this stateful test, now it's even more so - could we maybe cherry-pick @hodlinator's cleanup commit before of this change?