Bitcoin Core Github
44 subscribers
120K links
Download Telegram
👍 dergoegge approved a pull request: "test: Use msg_generic in p2p_ping.py"
(https://github.com/bitcoin/bitcoin/pull/32834#pullrequestreview-2971157730)
utACK fa3f100010f1a0273891ed0d14ddb3c5ccacbd4b
💬 romanz commented on pull request "rest: fetch spent transaction outputs by blockhash":
(https://github.com/bitcoin/bitcoin/pull/32540#discussion_r2175046245)
Fixed in #32825 (rebased to resolve a small conflict).
💬 romanz commented on pull request "index: store per-block transaction locations for efficient lookups":
(https://github.com/bitcoin/bitcoin/pull/32541#issuecomment-3019110833)
Rebased over `master` after https://github.com/bitcoin/bitcoin/pull/32825 is merged (to resolve a small conflict).
👍 theStack approved a pull request: "test: Use msg_generic in p2p_ping.py"
(https://github.com/bitcoin/bitcoin/pull/32834#pullrequestreview-2971207941)
ACK fa3f100010f1a0273891ed0d14ddb3c5ccacbd4b

Checked that this catches the issue fixed in #32833 as expected (i.e. p2p_ping.py fails if the `msgtype` slot is removed again from `msg_generic`).
💬 l0rinc commented on issue "[IBD] Raspberry Pi: 90% CPU time for 1.5% of block processing":
(https://github.com/bitcoin/bitcoin/issues/32832#issuecomment-3019159919)
> Could clarify in the title or in the text that this is for blocks after assumevalid?

Did I miscalculate it? Since the benchmark was running until 900k blocks of which only 13843 blocks needed script validation (unless I ran into https://github.com/bitcoin/bitcoin/issues/31494), isn't that 13843/900000=0.0153811111 i.e. 1.5% of the blocks had any script validation, yet almost 90% of the time was spent there?
🚀 fanquake merged a pull request: "test: Use msg_generic in p2p_ping.py"
(https://github.com/bitcoin/bitcoin/pull/32834)
💬 sipa commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#issuecomment-3019243840)
Would there be interest in adding a benchmark for the Trim() operation here, or rather in a follow-up?
💬 TheCharlatan commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2175160806)
More accurate error/standardness failure reporting can be done later through #29060.
💬 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
...
💬 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.
💬 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.
💬 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
...
👍 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.
💬 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;
```
💬 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. (#
...
💬 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.
💬 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?
📝 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
...
💬 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.
💬 TheCharlatan commented on pull request "test: fix feature_init.py intermittencies":
(https://github.com/bitcoin/bitcoin/pull/32835#issuecomment-3019500162)
Approach ACK