💬 ryanofsky commented on pull request "refactor: Remove CAddressBookData::destdata":
(https://github.com/bitcoin/bitcoin/pull/18608#issuecomment-1462387666)
#27224 is the rebased version of this PR. (I would have reopened this one but ran into github permission issues)
(https://github.com/bitcoin/bitcoin/pull/18608#issuecomment-1462387666)
#27224 is the rebased version of this PR. (I would have reopened this one but ran into github permission issues)
💬 ryanofsky commented on pull request "Blockstorage: Dont access gArgs to get blocks_dir":
(https://github.com/bitcoin/bitcoin/pull/27103#issuecomment-1462391839)
@TheCharlatan
Curious why this PR was closed, and what is its relationship to #27125? Is that a PR a replacement or superset or a different approach?
(https://github.com/bitcoin/bitcoin/pull/27103#issuecomment-1462391839)
@TheCharlatan
Curious why this PR was closed, and what is its relationship to #27125? Is that a PR a replacement or superset or a different approach?
💬 ryanofsky commented on issue "Permission to comment on closed PRs":
(https://github.com/bitcoin/bitcoin/issues/27234#issuecomment-1462409614)
Thanks for unlocking the PRs.
That is a good point that ability to comment on a locked PR is not really useful because relevant people won't be able to respond. Maybe ideally there would be a way to request that DrahtBot unlocks the PR, and then subsequently relocks it after a period of idle time (like a week or month).
Meantime, I guess I can use this issue to request things to be unlocked if the need arises. (At least until this issue itself is locked :grin:)
(https://github.com/bitcoin/bitcoin/issues/27234#issuecomment-1462409614)
Thanks for unlocking the PRs.
That is a good point that ability to comment on a locked PR is not really useful because relevant people won't be able to respond. Maybe ideally there would be a way to request that DrahtBot unlocks the PR, and then subsequently relocks it after a period of idle time (like a week or month).
Meantime, I guess I can use this issue to request things to be unlocked if the need arises. (At least until this issue itself is locked :grin:)
✅ ryanofsky closed an issue: "Permission to comment on closed PRs"
(https://github.com/bitcoin/bitcoin/issues/27234)
(https://github.com/bitcoin/bitcoin/issues/27234)
💬 TheCharlatan commented on pull request "Blockstorage: Dont access gArgs to get blocks_dir":
(https://github.com/bitcoin/bitcoin/pull/27103#issuecomment-1462411439)
> Curious why this PR was closed, and what is its relationship to https://github.com/bitcoin/bitcoin/pull/27125? Is that a PR a replacement or superset or a different approach?
I hit the wrong button by mistake :) - I should have left a comment, or better re-opened as a draft. The approach started here is the same though as in #27125.
(https://github.com/bitcoin/bitcoin/pull/27103#issuecomment-1462411439)
> Curious why this PR was closed, and what is its relationship to https://github.com/bitcoin/bitcoin/pull/27125? Is that a PR a replacement or superset or a different approach?
I hit the wrong button by mistake :) - I should have left a comment, or better re-opened as a draft. The approach started here is the same though as in #27125.
🚀 glozow merged a pull request: "github: Switch to yaml issue templates"
(https://github.com/bitcoin/bitcoin/pull/27025)
(https://github.com/bitcoin/bitcoin/pull/27025)
💬 Sjors commented on pull request "Add test and docs for getblockfrompeer with pruning":
(https://github.com/bitcoin/bitcoin/pull/23813#issuecomment-1462438231)
re-utACK fe329dc936d1e02da406345e4223e11d1fa6fb38
(https://github.com/bitcoin/bitcoin/pull/23813#issuecomment-1462438231)
re-utACK fe329dc936d1e02da406345e4223e11d1fa6fb38
💬 Sjors commented on pull request "assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/15606#issuecomment-1462444460)
CI trips over:
```
src/validation.cpp:5721:1:
error:
return type 'const ChainstateRole' is 'const'-qualified at the top level, which may reduce code readability without improving const correctness [readability-const-return-type,-warnings-as-errors]
```
(https://github.com/bitcoin/bitcoin/pull/15606#issuecomment-1462444460)
CI trips over:
```
src/validation.cpp:5721:1:
error:
return type 'const ChainstateRole' is 'const'-qualified at the top level, which may reduce code readability without improving const correctness [readability-const-return-type,-warnings-as-errors]
```
💬 TheCharlatan commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#issuecomment-1462445136)
Updated f87c39399823e22c553b797cc66fa4063462a32b -> 6d9826f182d9122d4464d35d6682dc6fb4b1116e ([removeBlockstorageArgs_6](https://github.com/TheCharlatan/bitcoin/tree/removeBlockstorageArgs_6) -> [removeBlockstorageArgs_7](https://github.com/TheCharlatan/bitcoin/tree/removeBlockstorageArgs_7), [compare](https://github.com/TheCharlatan/bitcoin/compare/removeBlockstorageArgs_6..removeBlockstorageArgs_7)) addressing https://github.com/bitcoin/bitcoin/pull/27125#pullrequestreview-1317942691 by insta
...
(https://github.com/bitcoin/bitcoin/pull/27125#issuecomment-1462445136)
Updated f87c39399823e22c553b797cc66fa4063462a32b -> 6d9826f182d9122d4464d35d6682dc6fb4b1116e ([removeBlockstorageArgs_6](https://github.com/TheCharlatan/bitcoin/tree/removeBlockstorageArgs_6) -> [removeBlockstorageArgs_7](https://github.com/TheCharlatan/bitcoin/tree/removeBlockstorageArgs_7), [compare](https://github.com/TheCharlatan/bitcoin/compare/removeBlockstorageArgs_6..removeBlockstorageArgs_7)) addressing https://github.com/bitcoin/bitcoin/pull/27125#pullrequestreview-1317942691 by insta
...
📝 dergoegge opened a pull request: "Avoid integer overflow in CheckDiskSpace"
(https://github.com/bitcoin/bitcoin/pull/27235)
Starting a fresh node with `-prune=1` causes an integer overflow to happen in `CheckDiskSpace` (e.g. https://cirrus-ci.com/task/5059018708746240) because `nPruneTarget` is to the max `uint64_t` value.
https://github.com/bitcoin/bitcoin/blob/f7bdcfc83f5753349018be3b5a663c8923d1a5eb/src/init.cpp#L1633-L1648
I think side stepping the overflow for this specific case, is better than adding an exception to the UB suppresions file. Alternatively we could use our overflow utils (e.g. `CheckedAdd`).
(https://github.com/bitcoin/bitcoin/pull/27235)
Starting a fresh node with `-prune=1` causes an integer overflow to happen in `CheckDiskSpace` (e.g. https://cirrus-ci.com/task/5059018708746240) because `nPruneTarget` is to the max `uint64_t` value.
https://github.com/bitcoin/bitcoin/blob/f7bdcfc83f5753349018be3b5a663c8923d1a5eb/src/init.cpp#L1633-L1648
I think side stepping the overflow for this specific case, is better than adding an exception to the UB suppresions file. Alternatively we could use our overflow utils (e.g. `CheckedAdd`).
💬 martinus commented on pull request "Avoid integer overflow in CheckDiskSpace":
(https://github.com/bitcoin/bitcoin/pull/27235#issuecomment-1462486422)
How about setting `nPruneTarget` to something smaller, like `std::numeric_limits<uint64_t>::max() / 2`? That's still large enough
(https://github.com/bitcoin/bitcoin/pull/27235#issuecomment-1462486422)
How about setting `nPruneTarget` to something smaller, like `std::numeric_limits<uint64_t>::max() / 2`? That's still large enough
💬 vasild commented on pull request "Avoid integer overflow in CheckDiskSpace":
(https://github.com/bitcoin/bitcoin/pull/27235#discussion_r1131394690)
A few issues:
1. This check uses the knowledge that the special magic constant `std::numeric_limits<uint64_t>::max()` is returned by `GetPruneTarget()` and that in such cases `additional_bytes` does not make sense. That's not for `CheckDiskSpace()` to know - the caller should choose a better `additional_bytes` when calling `CheckDiskSpace()`.
2. This magic constant is used in two other places - when setting `nPruneTarget` and in `LoadChainstate()`. It is better to give it a name instead of
...
(https://github.com/bitcoin/bitcoin/pull/27235#discussion_r1131394690)
A few issues:
1. This check uses the knowledge that the special magic constant `std::numeric_limits<uint64_t>::max()` is returned by `GetPruneTarget()` and that in such cases `additional_bytes` does not make sense. That's not for `CheckDiskSpace()` to know - the caller should choose a better `additional_bytes` when calling `CheckDiskSpace()`.
2. This magic constant is used in two other places - when setting `nPruneTarget` and in `LoadChainstate()`. It is better to give it a name instead of
...
💬 1440000bytes commented on pull request "p2p, validation: Don't download witnesses for assumed-valid blocks when running in prune mode":
(https://github.com/bitcoin/bitcoin/pull/27050#issuecomment-1462547782)
> It's important to recognize that this is a security reduction, even though it only applies to assumed-valid blocks: one of the reasons why assume-valid is acceptable, is because the data that we're assuming is valid is widely available for independent auditing and our peers can't distinguish between nodes that are and are not actually checking it. Skipping the downloading of witnesses negates that security by making it possible to sync a pruned node without anyone having that data available.
...
(https://github.com/bitcoin/bitcoin/pull/27050#issuecomment-1462547782)
> It's important to recognize that this is a security reduction, even though it only applies to assumed-valid blocks: one of the reasons why assume-valid is acceptable, is because the data that we're assuming is valid is widely available for independent auditing and our peers can't distinguish between nodes that are and are not actually checking it. Skipping the downloading of witnesses negates that security by making it possible to sync a pruned node without anyone having that data available.
...
💬 achow101 commented on pull request "wallet: Filter-out "send" addresses from `listreceivedby*`":
(https://github.com/bitcoin/bitcoin/pull/25973#issuecomment-1462547795)
ACK 62880a9bb167bc0569175d597a865e6baccce71b
(https://github.com/bitcoin/bitcoin/pull/25973#issuecomment-1462547795)
ACK 62880a9bb167bc0569175d597a865e6baccce71b
📝 dergoegge converted_to_draft a pull request: "Avoid integer overflow in CheckDiskSpace"
(https://github.com/bitcoin/bitcoin/pull/27235)
Starting a fresh node with `-prune=1` causes an integer overflow to happen in `CheckDiskSpace` (e.g. https://cirrus-ci.com/task/5059018708746240) because `nPruneTarget` is to the max `uint64_t` value.
https://github.com/bitcoin/bitcoin/blob/f7bdcfc83f5753349018be3b5a663c8923d1a5eb/src/init.cpp#L1633-L1648
I think side stepping the overflow for this specific case, is better than adding an exception to the UB suppresions file. Alternatively we could use our overflow utils (e.g. `CheckedAdd`).
(https://github.com/bitcoin/bitcoin/pull/27235)
Starting a fresh node with `-prune=1` causes an integer overflow to happen in `CheckDiskSpace` (e.g. https://cirrus-ci.com/task/5059018708746240) because `nPruneTarget` is to the max `uint64_t` value.
https://github.com/bitcoin/bitcoin/blob/f7bdcfc83f5753349018be3b5a663c8923d1a5eb/src/init.cpp#L1633-L1648
I think side stepping the overflow for this specific case, is better than adding an exception to the UB suppresions file. Alternatively we could use our overflow utils (e.g. `CheckedAdd`).
💬 jonatack commented on pull request "rpc: fix logging RPC when "none" values are passed, add test coverage, improve docs":
(https://github.com/bitcoin/bitcoin/pull/27231#issuecomment-1462594692)
The behavior was intended to be added in https://github.com/bitcoin/bitcoin/pull/11191.
I downloaded and tested v23.1 and the behavior wasn't functional: passing `0` or `none` had no effect.
In v24, f1379aeca9d3a8c4d3528de4d0af8298cb42fee4 made `none` not be recognized, but the logic for the intended behavior remained absent anyway.
I've added this information to the PR description.
(https://github.com/bitcoin/bitcoin/pull/27231#issuecomment-1462594692)
The behavior was intended to be added in https://github.com/bitcoin/bitcoin/pull/11191.
I downloaded and tested v23.1 and the behavior wasn't functional: passing `0` or `none` had no effect.
In v24, f1379aeca9d3a8c4d3528de4d0af8298cb42fee4 made `none` not be recognized, but the logic for the intended behavior remained absent anyway.
I've added this information to the PR description.
💬 jonatack commented on pull request "rpc: fix logging RPC when "none" values are passed, add test coverage, improve docs":
(https://github.com/bitcoin/bitcoin/pull/27231#discussion_r1131465027)
Thanks @MarcoFalke, done as follows.
```cpp
static void EnableOrDisableLogCategories(const UniValue& categories, bool enable)
{
const std::vector<UniValue>& category_values{categories.get_array().getValues()};
for (const auto& category : category_values) {
const std::string& c{category.get_str()};
if (c == "0" || c == "none") return; // no-op if one of these logging categories is passed
}
for (const auto& category : category_values) {
const std
...
(https://github.com/bitcoin/bitcoin/pull/27231#discussion_r1131465027)
Thanks @MarcoFalke, done as follows.
```cpp
static void EnableOrDisableLogCategories(const UniValue& categories, bool enable)
{
const std::vector<UniValue>& category_values{categories.get_array().getValues()};
for (const auto& category : category_values) {
const std::string& c{category.get_str()};
if (c == "0" || c == "none") return; // no-op if one of these logging categories is passed
}
for (const auto& category : category_values) {
const std
...
💬 jonatack commented on pull request "rpc: fix logging RPC when "none" values are passed, add test coverage, improve docs":
(https://github.com/bitcoin/bitcoin/pull/27231#discussion_r1131469401)
(Noting that this logic is similar to that in `SetLoggingCategories()` in `src/init/common.cpp`).
(https://github.com/bitcoin/bitcoin/pull/27231#discussion_r1131469401)
(Noting that this logic is similar to that in `SetLoggingCategories()` in `src/init/common.cpp`).
💬 amitiuttarwar commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/26988#issuecomment-1462609621)
light code review ACK 7c34c35b47. tested that the RPC and cli endpoints make sense & handle errors reasonably. these changes will require release notes, which can be done here or in a separate PR.
(https://github.com/bitcoin/bitcoin/pull/26988#issuecomment-1462609621)
light code review ACK 7c34c35b47. tested that the RPC and cli endpoints make sense & handle errors reasonably. these changes will require release notes, which can be done here or in a separate PR.
💬 amitiuttarwar commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1131449772)
you could add coverage for `getaddrmaninfo` with a network arg passed through
(https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1131449772)
you could add coverage for `getaddrmaninfo` with a network arg passed through