💬 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_r2185313756)
I think there are a number of issues with this part of the test:
- the docstring states locations 2 and 3 are exempt, but I don't see why they would be
- it tests that the location can log **up to** the ratelimit amount, which is true in any case
- it tests that the filesize increases, which would also happen if ratelimiting were to have happened, so it doesn't really catch anything
I think the `logging_filesize_rate_limit` test can be improved and cleaned up in a follow-up (to e.g. addres
...
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2185313756)
I think there are a number of issues with this part of the test:
- the docstring states locations 2 and 3 are exempt, but I don't see why they would be
- it tests that the location can log **up to** the ratelimit amount, which is true in any case
- it tests that the filesize increases, which would also happen if ratelimiting were to have happened, so it doesn't really catch anything
I think the `logging_filesize_rate_limit` test can be improved and cleaned up in a follow-up (to e.g. addres
...
💬 hodlinator commented on pull request "test: fix feature_init.py intermittencies":
(https://github.com/bitcoin/bitcoin/pull/32835#discussion_r2185335801)
Oof, that's nasty.
(https://github.com/bitcoin/bitcoin/pull/32835#discussion_r2185335801)
Oof, that's nasty.
💬 hodlinator commented on pull request "test: fix feature_init.py intermittencies":
(https://github.com/bitcoin/bitcoin/pull/32835#discussion_r2185355403)
If we know for sure, why are we running `check_clean_start()`? IMO we should do one of:
* Trust that we fully restored the state and remove `check_clean_start()` here.
* Run `check_clean_start(startup_args)` to verify that the index being tested was restored.
(https://github.com/bitcoin/bitcoin/pull/32835#discussion_r2185355403)
If we know for sure, why are we running `check_clean_start()`? IMO we should do one of:
* Trust that we fully restored the state and remove `check_clean_start()` here.
* Run `check_clean_start(startup_args)` to verify that the index being tested was restored.
💬 Sjors commented on pull request "wallet: allow skipping script paths":
(https://github.com/bitcoin/bitcoin/pull/32857#issuecomment-3036332191)
I opened #32876 and rebased this PR on top of it. That allowed me to drop 1486f05c8eded02d72690bf34d983a03e4ad748b + efc34a514b59cc1bab6e4d0f80c197481561c429.
(https://github.com/bitcoin/bitcoin/pull/32857#issuecomment-3036332191)
I opened #32876 and rebased this PR on top of it. That allowed me to drop 1486f05c8eded02d72690bf34d983a03e4ad748b + efc34a514b59cc1bab6e4d0f80c197481561c429.
💬 Eunovo commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2185423052)
For this test, I need the `minimumchainwork` to be higher than the best_header chainwork. The `height` of the best_header here is `3714`, so the `minimumchainwork` needs to be for a chain considerably higher than `3714` blocks. Antoine had provided the exact chain work for `32256` blocks in this [stack exchange answer](https://bitcoin.stackexchange.com/questions/26869/what-is-chainwork#66874), so I used that.
Note that the `minimumchainwork` used doesn't affect the duration of the test, it
...
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2185423052)
For this test, I need the `minimumchainwork` to be higher than the best_header chainwork. The `height` of the best_header here is `3714`, so the `minimumchainwork` needs to be for a chain considerably higher than `3714` blocks. Antoine had provided the exact chain work for `32256` blocks in this [stack exchange answer](https://bitcoin.stackexchange.com/questions/26869/what-is-chainwork#66874), so I used that.
Note that the `minimumchainwork` used doesn't affect the duration of the test, it
...
⚠️ brunoerg opened an issue: "test: break `feature_block` into subtests?"
(https://github.com/bitcoin/bitcoin/issues/32877)
According to [corecheck](https://corecheck.dev/tests), `feature_block` is the slowest functional test and it's taking about 70 seconds to run. `feature_block` is a huge functional test that tests many things - large re-orgs, block with invalid transactions, disabled opcodes, sigop counts, etc. These things are not necessary coupled and could be broken into subtests and then we could run some of them instead of the whole test (see https://github.com/bitcoin/bitcoin/pull/30991) when necessary.
T
...
(https://github.com/bitcoin/bitcoin/issues/32877)
According to [corecheck](https://corecheck.dev/tests), `feature_block` is the slowest functional test and it's taking about 70 seconds to run. `feature_block` is a huge functional test that tests many things - large re-orgs, block with invalid transactions, disabled opcodes, sigop counts, etc. These things are not necessary coupled and could be broken into subtests and then we could run some of them instead of the whole test (see https://github.com/bitcoin/bitcoin/pull/30991) when necessary.
T
...
💬 furszy commented on pull request "test: fix feature_init.py intermittencies":
(https://github.com/bitcoin/bitcoin/pull/32835#discussion_r2185424642)
> If we know for sure, why are we running check_clean_start()?
I assume we are doing it to verify nothing else broke after deleting + restoring some data. But you have a fair point.
Pushed.
(https://github.com/bitcoin/bitcoin/pull/32835#discussion_r2185424642)
> If we know for sure, why are we running check_clean_start()?
I assume we are doing it to verify nothing else broke after deleting + restoring some data. But you have a fair point.
Pushed.
💬 Eunovo commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#issuecomment-3036343118)
Rebased https://github.com/bitcoin/bitcoin/pull/31615/commits/11a9d55e84af2d6ff138b99310720667cd59dcc3 to https://github.com/bitcoin/bitcoin/pull/31615/commits/e260e23d88cd6b2aaa03d9d45dae8b174f2cfe1f
- moved `feature_reindex.py` test to "Tests less than 2m" section in `test_runner.py`
(https://github.com/bitcoin/bitcoin/pull/31615#issuecomment-3036343118)
Rebased https://github.com/bitcoin/bitcoin/pull/31615/commits/11a9d55e84af2d6ff138b99310720667cd59dcc3 to https://github.com/bitcoin/bitcoin/pull/31615/commits/e260e23d88cd6b2aaa03d9d45dae8b174f2cfe1f
- moved `feature_reindex.py` test to "Tests less than 2m" section in `test_runner.py`
💬 Eunovo commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2185441225)
@stratospher the test completes much faster on my system, but by checking the timing of other tests, I put it among "Tests less than 2m" section because it completes faster than `mining_getblocktemplate_longpoll.py` than slower than `mempool_persist.py`
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2185441225)
@stratospher the test completes much faster on my system, but by checking the timing of other tests, I put it among "Tests less than 2m" section because it completes faster than `mining_getblocktemplate_longpoll.py` than slower than `mempool_persist.py`
💬 maflcko commented on issue "test: break `feature_block` into subtests?":
(https://github.com/bitcoin/bitcoin/issues/32877#issuecomment-3036407180)
> These things are not necessary coupled
Are you sure? I'd say the reorg test is useful to perform on a "dirty" state.
Other than that, I think the reorg test is the slowest part of the test, so if you want to run the test twice (once with reorg and once without reorg), it should be fine and also solve your issue?
(https://github.com/bitcoin/bitcoin/issues/32877#issuecomment-3036407180)
> These things are not necessary coupled
Are you sure? I'd say the reorg test is useful to perform on a "dirty" state.
Other than that, I think the reorg test is the slowest part of the test, so if you want to run the test twice (once with reorg and once without reorg), it should be fine and also solve your issue?
🤔 stickies-v reviewed a pull request: "index: Fix missing case in the comment in NextSyncBlock()"
(https://github.com/bitcoin/bitcoin/pull/32875#pullrequestreview-2987509048)
You're correct that the current docstring is incorrect. I personally don't find your suggestion very clear either, as the two cases talk about different parts of the return statement.
An alternative would be to keep the docstring as-is and just make the chaintip case more explicit in code:
```cpp
const CBlockIndex* pindex = chain.Next(pindex_prev);
if (pindex || pindex_prev == chain.Tip()) {
return pindex;
}
```
<details>
<summary>git diff on d6bf3b1cc2</summar
...
(https://github.com/bitcoin/bitcoin/pull/32875#pullrequestreview-2987509048)
You're correct that the current docstring is incorrect. I personally don't find your suggestion very clear either, as the two cases talk about different parts of the return statement.
An alternative would be to keep the docstring as-is and just make the chaintip case more explicit in code:
```cpp
const CBlockIndex* pindex = chain.Next(pindex_prev);
if (pindex || pindex_prev == chain.Tip()) {
return pindex;
}
```
<details>
<summary>git diff on d6bf3b1cc2</summar
...
💬 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#issuecomment-3036620662)
> Left a few nits/test comments that can be done in a follow-up
I will make a follow-up PR post-merge to address the comments (which are all reasonable, just want to limit re-review)
(https://github.com/bitcoin/bitcoin/pull/32604#issuecomment-3036620662)
> Left a few nits/test comments that can be done in a follow-up
I will make a follow-up PR post-merge to address the comments (which are all reasonable, just want to limit re-review)
💬 stratospher commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2185605623)
ah I see, but that's on mainnet and not on regtest, so comment might need an edit. agree that the value doesn't affect the test!
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2185605623)
ah I see, but that's on mainnet and not on regtest, so comment might need an edit. agree that the value doesn't affect the test!
💬 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_r2185605781)
I had thought about that as well. I looked for patterns like this, but I did not look very hard. Do you think this deserves a comment in a follow-up to warn against behavior like this?
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2185605781)
I had thought about that as well. I looked for patterns like this, but I did not look very hard. Do you think this deserves a comment in a follow-up to warn against behavior like this?
💬 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_r2185609148)
Will address in a follow-up, I think this is from an older version of the PR where the functions used in location 2/3 were not caught by the rate limit.
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2185609148)
Will address in a follow-up, I think this is from an older version of the PR where the functions used in location 2/3 were not caught by the rate limit.
💬 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_r2185611861)
`Stats` is a better name, `LogRateLimiter::LogLimitStats` is a bit verbose / stutter-y. Will address in a follow-up, I am not so good with naming.
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2185611861)
`Stats` is a better name, `LogRateLimiter::LogLimitStats` is a bit verbose / stutter-y. Will address in a follow-up, I am not so good with naming.
💬 HowHsu commented on pull request "index: Fix missing case in the comment in NextSyncBlock()":
(https://github.com/bitcoin/bitcoin/pull/32875#issuecomment-3036657452)
> You're correct that the current docstring is incorrect. I personally don't find your suggestion very clear either, as the two cases talk about different parts of the return statement.
>
> An alternative would be to keep the docstring as-is and just make the chaintip case more explicit in code:
>
> ```c++
> const CBlockIndex* pindex = chain.Next(pindex_prev);
> if (pindex || pindex_prev == chain.Tip()) {
> return pindex;
> }
> ```
>
> git diff on [d6bf3b1](http
...
(https://github.com/bitcoin/bitcoin/pull/32875#issuecomment-3036657452)
> You're correct that the current docstring is incorrect. I personally don't find your suggestion very clear either, as the two cases talk about different parts of the return statement.
>
> An alternative would be to keep the docstring as-is and just make the chaintip case more explicit in code:
>
> ```c++
> const CBlockIndex* pindex = chain.Next(pindex_prev);
> if (pindex || pindex_prev == chain.Tip()) {
> return pindex;
> }
> ```
>
> git diff on [d6bf3b1](http
...
💬 waketraindev commented on pull request "rpc: add optional nodeid param to filter getpeerinfo":
(https://github.com/bitcoin/bitcoin/pull/32741#discussion_r2185644531)
Added line to help text, preferred using peer id naming instead of index, to avoid confusion with the array index
(https://github.com/bitcoin/bitcoin/pull/32741#discussion_r2185644531)
Added line to help text, preferred using peer id naming instead of index, to avoid confusion with the array index
💬 waketraindev commented on pull request "rpc: add optional nodeid param to filter getpeerinfo":
(https://github.com/bitcoin/bitcoin/pull/32741#discussion_r2185650755)
First condition checks if the parameter is defined so it returns all results even if peer_id is 0
(https://github.com/bitcoin/bitcoin/pull/32741#discussion_r2185650755)
First condition checks if the parameter is defined so it returns all results even if peer_id is 0
💬 waketraindev commented on pull request "rpc: add optional nodeid param to filter getpeerinfo":
(https://github.com/bitcoin/bitcoin/pull/32741#issuecomment-3036685462)
Rebased on top of master, updated help text
(https://github.com/bitcoin/bitcoin/pull/32741#issuecomment-3036685462)
Rebased on top of master, updated help text