💬 glozow commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#issuecomment-3053633853)
reACK 4c772cb
(https://github.com/bitcoin/bitcoin/pull/32604#issuecomment-3053633853)
reACK 4c772cb
🚀 glozow merged a pull request: "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError"
(https://github.com/bitcoin/bitcoin/pull/32604)
(https://github.com/bitcoin/bitcoin/pull/32604)
💬 instagibbs commented on pull request "p2p: Relax BlockRequestAllowed to respond to advertised blocks":
(https://github.com/bitcoin/bitcoin/pull/32869#issuecomment-3053653904)
@davidgumberg for completeness, the other solution, as stated in the issue tracker, would be to send a `notfound` in response to requests for invalid blocks. I'm not sure where people landed on that as that discussion was a long time ago
(https://github.com/bitcoin/bitcoin/pull/32869#issuecomment-3053653904)
@davidgumberg for completeness, the other solution, as stated in the issue tracker, would be to send a `notfound` in response to requests for invalid blocks. I'm not sure where people landed on that as that discussion was a long time ago
💬 pablomartin4btc commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2195750958)
I could use a better naming for this function, not being an overload. My idea was to fix the issue without touching much and later refactor both `GetWalletNameFromJSONRPCRequest` and `GetWalletForJSONRPCRequest` on a separate PR. Currently `GetWalletForJSONRPCRequest`, which is used broadly, calls internally `GetWalletNameFromJSONRPCRequest` and wanted to keep them untouched for now.
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2195750958)
I could use a better naming for this function, not being an overload. My idea was to fix the issue without touching much and later refactor both `GetWalletNameFromJSONRPCRequest` and `GetWalletForJSONRPCRequest` on a separate PR. Currently `GetWalletForJSONRPCRequest`, which is used broadly, calls internally `GetWalletNameFromJSONRPCRequest` and wanted to keep them untouched for now.
💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2195765195)
I prefer it this way.
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2195765195)
I prefer it this way.
💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2195765353)
Done.
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2195765353)
Done.
💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2195765533)
Done.
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2195765533)
Done.
💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2195766179)
Done. Thanks for the screenshot.
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2195766179)
Done. Thanks for the screenshot.
🚀 glozow merged a pull request: "mempool: use `FeeFrac` for ancestor/descendant score comparators"
(https://github.com/bitcoin/bitcoin/pull/32799)
(https://github.com/bitcoin/bitcoin/pull/32799)
💬 ishaanam commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2195916152)
I've changed this test to import `TX_MAX_STANDARD_VERSION` instead.
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2195916152)
I've changed this test to import `TX_MAX_STANDARD_VERSION` instead.
💬 ishaanam commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2195917838)
Done
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2195917838)
Done
🤔 furszy reviewed a pull request: "index: fix wrong assert of current_tip == m_best_block_index"
(https://github.com/bitcoin/bitcoin/pull/32878#pullrequestreview-3003004554)
@HowHsu, check this test https://github.com/furszy/bitcoin-core/commit/f2b8a06060e2a313f35101454c7669d46e0edc38.
It triggers the reorg assertion failure without needing to deduplicate the index synchronization code. The test fails without your fix commit and passes with it. Feel free to cherry-pick it.
> The Sync in the test is same as in bitcoind, expect the thread synchronization code which is to trigger the reorg. The only cons is we have to keep it updated when the logic of Sync() change
...
(https://github.com/bitcoin/bitcoin/pull/32878#pullrequestreview-3003004554)
@HowHsu, check this test https://github.com/furszy/bitcoin-core/commit/f2b8a06060e2a313f35101454c7669d46e0edc38.
It triggers the reorg assertion failure without needing to deduplicate the index synchronization code. The test fails without your fix commit and passes with it. Feel free to cherry-pick it.
> The Sync in the test is same as in bitcoind, expect the thread synchronization code which is to trigger the reorg. The only cons is we have to keep it updated when the logic of Sync() change
...
💬 achow101 commented on pull request "wallet: Track no-longer-spendable TXOs separately":
(https://github.com/bitcoin/bitcoin/pull/27865#discussion_r2195947572)
Using a default value has the potential very problematic because its value is expected to be correct. By using an optional, we can detect when there is a programming error that did not set the value.
(https://github.com/bitcoin/bitcoin/pull/27865#discussion_r2195947572)
Using a default value has the potential very problematic because its value is expected to be correct. By using an optional, we can detect when there is a programming error that did not set the value.
💬 sipa commented on pull request "Introduce per-txin sighash midstate cache for legacy/p2sh/segwitv0 scripts":
(https://github.com/bitcoin/bitcoin/pull/32473#discussion_r2195958660)
Fixed.
(https://github.com/bitcoin/bitcoin/pull/32473#discussion_r2195958660)
Fixed.
💬 sipa commented on pull request "Introduce per-txin sighash midstate cache for legacy/p2sh/segwitv0 scripts":
(https://github.com/bitcoin/bitcoin/pull/32473#issuecomment-3054002498)
@darosior I did it slightly differently, but added a commit that makes the use of the sighash cache optional. I avoided using default values as they can result in unexpected/missed call sites.
(https://github.com/bitcoin/bitcoin/pull/32473#issuecomment-3054002498)
@darosior I did it slightly differently, but added a commit that makes the use of the sighash cache optional. I avoided using default values as they can result in unexpected/missed call sites.
💬 davidgumberg commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#issuecomment-3054117201)
Post-merge ACK https://github.com/bitcoin/bitcoin/commit/4c772cbd83e502a1339e8993d192ea6416ecd45c
Lightly reviewed code, tested by running IBD with `debug=all` and `-nodebug`, no messages suppressed, ran logging related benchmarks and measured performance impact is minimal.
One more really minor and feel-free-to-disregard nit for the follow-up pile:
The `LOG_TIME*` macros incorrectly report source locations from `logging/timer.h` rather than where the macros are invoked. There are curr
...
(https://github.com/bitcoin/bitcoin/pull/32604#issuecomment-3054117201)
Post-merge ACK https://github.com/bitcoin/bitcoin/commit/4c772cbd83e502a1339e8993d192ea6416ecd45c
Lightly reviewed code, tested by running IBD with `debug=all` and `-nodebug`, no messages suppressed, ran logging related benchmarks and measured performance impact is minimal.
One more really minor and feel-free-to-disregard nit for the follow-up pile:
The `LOG_TIME*` macros incorrectly report source locations from `logging/timer.h` rather than where the macros are invoked. There are curr
...
🚀 achow101 merged a pull request: "wallet, test: best block locator matches scan state follow-ups"
(https://github.com/bitcoin/bitcoin/pull/32580)
(https://github.com/bitcoin/bitcoin/pull/32580)
📝 hodlinator opened a pull request: "qa: Avoid knock-on exception in assert_start_raises_init_error"
(https://github.com/bitcoin/bitcoin/pull/32929)
Raising a new exception from within a Python `except`-block causes the interpreter to generate extra error output which is unnecessary in this case.
Found while testing #32835 using the suggested method (https://github.com/bitcoin/bitcoin/pull/32835#issue-3188748624) which triggered expected timeouts, but with the extra error noise.
<details><summary>Example output before & after this PR</summary>
Before:
```
2025-07-08T20:05:48.407001Z TestFramework (ERROR): Assertion failed
Traceba
...
(https://github.com/bitcoin/bitcoin/pull/32929)
Raising a new exception from within a Python `except`-block causes the interpreter to generate extra error output which is unnecessary in this case.
Found while testing #32835 using the suggested method (https://github.com/bitcoin/bitcoin/pull/32835#issue-3188748624) which triggered expected timeouts, but with the extra error noise.
<details><summary>Example output before & after this PR</summary>
Before:
```
2025-07-08T20:05:48.407001Z TestFramework (ERROR): Assertion failed
Traceba
...
💬 mzumsande commented on pull request "index: fix wrong assert of current_tip == m_best_block_index":
(https://github.com/bitcoin/bitcoin/pull/32878#issuecomment-3054182296)
General question - should we remove the assert, or would it maybe be better to keep it, but commit the best block index before calling `Rewind()` in `Sync()`?
(https://github.com/bitcoin/bitcoin/pull/32878#issuecomment-3054182296)
General question - should we remove the assert, or would it maybe be better to keep it, but commit the best block index before calling `Rewind()` in `Sync()`?
💬 pablomartin4btc commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2196069916)
Thanks for the follow-up. The current fix is wrong. Looking at the original `getdescriptoractivity` [implementation](https://github.com/bitcoin/bitcoin/pull/30708) PR samples and tests, also checking the logic in the RPC, the second array, descriptors, has to be provided. Now, the first one, blockhashes, could be empty and activities may be returned in result if there are any in the mempool (3rd parameter `include_mempool` is true by default, but if the user sets it to false, the RPC is still us
...
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2196069916)
Thanks for the follow-up. The current fix is wrong. Looking at the original `getdescriptoractivity` [implementation](https://github.com/bitcoin/bitcoin/pull/30708) PR samples and tests, also checking the logic in the RPC, the second array, descriptors, has to be provided. Now, the first one, blockhashes, could be empty and activities may be returned in result if there are any in the mempool (3rd parameter `include_mempool` is true by default, but if the user sets it to false, the RPC is still us
...