💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-3019305155)
> The `bench.run` call does not clear `coins`, so it's warm after the first iteration.
Actually the benchmark does populate the cache first through `SetupDummyInputs` (otherwise it couldn't work since it is using a dummy backend). But the main point really is that this micro benchmark is meaningless to demonstrate a slow down in transaction processing. `AreInputsStandard` should represent a fraction of a percent of the total work performed when processing a transaction. Unless we make it 10'0
...
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-3019305155)
> The `bench.run` call does not clear `coins`, so it's warm after the first iteration.
Actually the benchmark does populate the cache first through `SetupDummyInputs` (otherwise it couldn't work since it is using a dummy backend). But the main point really is that this micro benchmark is meaningless to demonstrate a slow down in transaction processing. `AreInputsStandard` should represent a fraction of a percent of the total work performed when processing a transaction. Unless we make it 10'0
...
💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2175175517)
Apologies if that came across as patronizing, this was not my intention. That said, you should expect to receive pushback if you insistently derail a PR with irrelevant concerns.
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2175175517)
Apologies if that came across as patronizing, this was not my intention. That said, you should expect to receive pushback if you insistently derail a PR with irrelevant concerns.
💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2175176935)
I think the code is fine as-is, i don't plan on doing anything else here so will resolve this thread.
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2175176935)
I think the code is fine as-is, i don't plan on doing anything else here so will resolve this thread.
💬 stickies-v commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2175129044)
nit:
log strings currently look like:
> Restarting logging from ../../src/rpc/request.cpp:140 (GenerateAuthCookieResult GenerateAuthCookie(const std::optional<fs::perms> &, std::string &, std::string &)): **(0 MiB)** were dropped during the last hour.
Would suggest printing this as bytes and removing the parentheses.
```suggestion
"%d bytes were dropped during the last hour.\n",
source_loc.file_name(), source_l
...
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2175129044)
nit:
log strings currently look like:
> Restarting logging from ../../src/rpc/request.cpp:140 (GenerateAuthCookieResult GenerateAuthCookie(const std::optional<fs::perms> &, std::string &, std::string &)): **(0 MiB)** were dropped during the last hour.
Would suggest printing this as bytes and removing the parentheses.
```suggestion
"%d bytes were dropped during the last hour.\n",
source_loc.file_name(), source_l
...
👍 stickies-v approved a pull request: "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError"
(https://github.com/bitcoin/bitcoin/pull/32604#pullrequestreview-2971317445)
ACK 2ac8696b53e455dd27c8341828404a23b5cb68a9
I still would prefer the approach I suggested [here](https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2172385738) (and am happy to re-review), but I think the PR is fine as is now too.
(https://github.com/bitcoin/bitcoin/pull/32604#pullrequestreview-2971317445)
ACK 2ac8696b53e455dd27c8341828404a23b5cb68a9
I still would prefer the approach I suggested [here](https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2172385738) (and am happy to re-review), but I think the PR is fine as is now too.
💬 stickies-v commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2175159834)
nit
```suggestion
std::string str threadname;
```
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2175159834)
nit
```suggestion
std::string str threadname;
```
💬 stickies-v commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2175213743)
nit: the `-logsourcelocation` behaviour change is unrelated to the rate limiting, so I think this phrasing is a bit confusing. Would put it in a separate paragraph:
```
Unconditional logging to disk via LogPrintf, LogInfo, LogWarning, LogError, and
LogPrintLevel is now rate limited by giving each source location a logging quota of
1MiB per hour. (#32604)
When `-logsourcelocation` is enabled, the log output now contains the entire function signature
instead of just the function name. (#
...
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2175213743)
nit: the `-logsourcelocation` behaviour change is unrelated to the rate limiting, so I think this phrasing is a bit confusing. Would put it in a separate paragraph:
```
Unconditional logging to disk via LogPrintf, LogInfo, LogWarning, LogError, and
LogPrintLevel is now rate limited by giving each source location a logging quota of
1MiB per hour. (#32604)
When `-logsourcelocation` is enabled, the log output now contains the entire function signature
instead of just the function name. (#
...
💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2175235373)
That this does not touch consensus can be ensured through code review and is also validated through a functional test. I don't think anything else is necessary to do here, so i'm going to resolve this thread.
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2175235373)
That this does not touch consensus can be ensured through code review and is also validated through a functional test. I don't think anything else is necessary to do here, so i'm going to resolve this thread.
💬 TheCharlatan commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2175236097)
Could we just skip signing them?
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2175236097)
Could we just skip signing them?
📝 furszy opened a pull request: "test: fix feature_init.py intermittencies "
(https://github.com/bitcoin/bitcoin/pull/32835)
Aims to solve #32600. Found it while working on #26966 (this was really annoying there).
This ensures the node is index-synced before perturbing files.
If the index sync gets interrupted before it starts, the database could be empty,
making any following perturbation ineffective (which explains why the node
does not abort during startup in the #32600 logs).
Also, the first commit avoids initializing components not under test.
This reduces log flooding, which helped in understanding the
...
(https://github.com/bitcoin/bitcoin/pull/32835)
Aims to solve #32600. Found it while working on #26966 (this was really annoying there).
This ensures the node is index-synced before perturbing files.
If the index sync gets interrupted before it starts, the database could be empty,
making any following perturbation ineffective (which explains why the node
does not abort during startup in the #32600 logs).
Also, the first commit avoids initializing components not under test.
This reduces log flooding, which helped in understanding the
...
💬 furszy commented on issue "intermittent issue in feature_init.py (bitcoind should have exited with expected error LevelDB error: Corruption)":
(https://github.com/bitcoin/bitcoin/issues/32600#issuecomment-3019459837)
#32835.
(https://github.com/bitcoin/bitcoin/issues/32600#issuecomment-3019459837)
#32835.
💬 TheCharlatan commented on pull request "test: fix feature_init.py intermittencies":
(https://github.com/bitcoin/bitcoin/pull/32835#issuecomment-3019500162)
Approach ACK
(https://github.com/bitcoin/bitcoin/pull/32835#issuecomment-3019500162)
Approach ACK
💬 maflcko commented on pull request "test: fix feature_init.py intermittencies":
(https://github.com/bitcoin/bitcoin/pull/32835#discussion_r2175309533)
any reason to start the node again? Can't this wait_until be done in check_clean_start?
(https://github.com/bitcoin/bitcoin/pull/32835#discussion_r2175309533)
any reason to start the node again? Can't this wait_until be done in check_clean_start?
⚠️ Ciornenichii opened an issue: "Ciornenichii stepan"
(https://github.com/bitcoin/bitcoin/issues/32836)
(https://github.com/bitcoin/bitcoin/issues/32836)
✅ fanquake closed an issue: "Ciornenichii stepan"
(https://github.com/bitcoin/bitcoin/issues/32836)
(https://github.com/bitcoin/bitcoin/issues/32836)
💬 Crypt-iQ commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2175338142)
Leaving a note that I am going to implement these changes and plan to push up either later today or tomorrow.
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2175338142)
Leaving a note that I am going to implement these changes and plan to push up either later today or tomorrow.
💬 instagibbs commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#issuecomment-3019599266)
@dergoegge awesome coverage and functional test!
Seems this is the issue, we are `visit`ing children erroneously disallowing their removal later when parent is `visit`ed for immaturity:
```
diff --git a/src/txmempool.cpp b/src/txmempool.cpp
index 5c90bc43dd..3c278f6aad 100644
--- a/src/txmempool.cpp
+++ b/src/txmempool.cpp
@@ -362,5 +362,5 @@ void CTxMemPool::removeForReorg(CChain& chain, std::function<bool(txiter)> check
WITH_FRESH_EPOCH(m_epoch);
for (indexed_trans
...
(https://github.com/bitcoin/bitcoin/pull/28676#issuecomment-3019599266)
@dergoegge awesome coverage and functional test!
Seems this is the issue, we are `visit`ing children erroneously disallowing their removal later when parent is `visit`ed for immaturity:
```
diff --git a/src/txmempool.cpp b/src/txmempool.cpp
index 5c90bc43dd..3c278f6aad 100644
--- a/src/txmempool.cpp
+++ b/src/txmempool.cpp
@@ -362,5 +362,5 @@ void CTxMemPool::removeForReorg(CChain& chain, std::function<bool(txiter)> check
WITH_FRESH_EPOCH(m_epoch);
for (indexed_trans
...
💬 instagibbs commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#issuecomment-3019618093)
@sipa linked here, added to follow-up potentially?
(https://github.com/bitcoin/bitcoin/pull/31553#issuecomment-3019618093)
@sipa linked here, added to follow-up potentially?
💬 furszy commented on pull request "test: fix feature_init.py intermittencies":
(https://github.com/bitcoin/bitcoin/pull/32835#discussion_r2175362595)
> any reason to start the node again? Can't this wait_until be done in check_clean_start?
Just because `check_clean_start()` is also called inside the deletion rounds and didn't want to start all indexes there on each cycle round. But that seems easily solvable.. one minute..
(https://github.com/bitcoin/bitcoin/pull/32835#discussion_r2175362595)
> any reason to start the node again? Can't this wait_until be done in check_clean_start?
Just because `check_clean_start()` is also called inside the deletion rounds and didn't want to start all indexes there on each cycle round. But that seems easily solvable.. one minute..
💬 furszy commented on pull request "test: fix feature_init.py intermittencies":
(https://github.com/bitcoin/bitcoin/pull/32835#discussion_r2175376795)
Done.
(https://github.com/bitcoin/bitcoin/pull/32835#discussion_r2175376795)
Done.