💬 MarcoFalke commented on pull request "github: Switch to yaml issue templates":
(https://github.com/bitcoin/bitcoin/pull/27025#discussion_r1132224642)
Looks like this wasn't addressed?
(https://github.com/bitcoin/bitcoin/pull/27025#discussion_r1132224642)
Looks like this wasn't addressed?
💬 vasild commented on pull request "Avoid integer overflow in CheckDiskSpace":
(https://github.com/bitcoin/bitcoin/pull/27235#issuecomment-1463633762)
Just for reference, like @MarcoFalke pointed above:
```
node1 stderr util/system.cpp:138:51: runtime error: unsigned integer overflow: 52428800 + 18446744073709551615 cannot be represented in type 'unsigned long'
#0 0x564a482b5088 in CheckDiskSpace(fs::path const&, unsigned long) src/./src/util/system.cpp:138:51
#1 0x564a4728dc59 in AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*) src/./src/init.cpp:1639:14
#2 0x564a47256e6a in AppInit(node::NodeContext&, i
...
(https://github.com/bitcoin/bitcoin/pull/27235#issuecomment-1463633762)
Just for reference, like @MarcoFalke pointed above:
```
node1 stderr util/system.cpp:138:51: runtime error: unsigned integer overflow: 52428800 + 18446744073709551615 cannot be represented in type 'unsigned long'
#0 0x564a482b5088 in CheckDiskSpace(fs::path const&, unsigned long) src/./src/util/system.cpp:138:51
#1 0x564a4728dc59 in AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*) src/./src/init.cpp:1639:14
#2 0x564a47256e6a in AppInit(node::NodeContext&, i
...
📝 TheCharlatan opened a pull request: "refactor: Split logging utilities from system.h"
(https://github.com/bitcoin/bitcoin/pull/27238)
This pull request is part of the `libbitcoinkernel` project https://github.com/bitcoin/bitcoin/issues/24303 https://github.com/bitcoin/bitcoin/projects/18 and more specifically its "Step 2: Decouple most non-consensus code from libbitcoinkernel". These commits were originally authored by @empact and are taken from their parent PR #25152.
#### Context
There is an ongoing effort to decouple the `ArgsManager` used for command line parsing user-provided arguments from the libbitcoinkernel libr
...
(https://github.com/bitcoin/bitcoin/pull/27238)
This pull request is part of the `libbitcoinkernel` project https://github.com/bitcoin/bitcoin/issues/24303 https://github.com/bitcoin/bitcoin/projects/18 and more specifically its "Step 2: Decouple most non-consensus code from libbitcoinkernel". These commits were originally authored by @empact and are taken from their parent PR #25152.
#### Context
There is an ongoing effort to decouple the `ArgsManager` used for command line parsing user-provided arguments from the libbitcoinkernel libr
...
💬 vasild commented on pull request "Avoid integer overflow in CheckDiskSpace":
(https://github.com/bitcoin/bitcoin/pull/27235#discussion_r1132241660)
Do you mean `SaturatingAdd(min_disk_space, additional_bytes)`? If yes, then in this case it would end up with `UINT64_MAX` and this function will always report that there is not enough disk space (because that number is so big, nobody has so much disk space).
In this case, I think neither `0` or `UINT64_MAX` is suitable. In manual pruning mode, I guess, it makes sense to assume that the user will delete some blocks but not much and thus the required disk space is as if in non-pruned mode - `A
...
(https://github.com/bitcoin/bitcoin/pull/27235#discussion_r1132241660)
Do you mean `SaturatingAdd(min_disk_space, additional_bytes)`? If yes, then in this case it would end up with `UINT64_MAX` and this function will always report that there is not enough disk space (because that number is so big, nobody has so much disk space).
In this case, I think neither `0` or `UINT64_MAX` is suitable. In manual pruning mode, I guess, it makes sense to assume that the user will delete some blocks but not much and thus the required disk space is as if in non-pruned mode - `A
...
💬 MarcoFalke commented on pull request "Avoid integer overflow in CheckDiskSpace":
(https://github.com/bitcoin/bitcoin/pull/27235#discussion_r1132249006)
Ah, correct. Sorry for my wrong review comment.
(https://github.com/bitcoin/bitcoin/pull/27235#discussion_r1132249006)
Ah, correct. Sorry for my wrong review comment.
📝 TheCharlatan converted_to_draft a pull request: "refactor: Split logging utilities from system.h"
(https://github.com/bitcoin/bitcoin/pull/27238)
This pull request is part of the `libbitcoinkernel` project https://github.com/bitcoin/bitcoin/issues/24303 https://github.com/bitcoin/bitcoin/projects/18 and more specifically its "Step 2: Decouple most non-consensus code from libbitcoinkernel". These commits were originally authored by @empact and are taken from their parent PR #25152.
#### Context
There is an ongoing effort to decouple the `ArgsManager` used for command line parsing user-provided arguments from the libbitcoinkernel libr
...
(https://github.com/bitcoin/bitcoin/pull/27238)
This pull request is part of the `libbitcoinkernel` project https://github.com/bitcoin/bitcoin/issues/24303 https://github.com/bitcoin/bitcoin/projects/18 and more specifically its "Step 2: Decouple most non-consensus code from libbitcoinkernel". These commits were originally authored by @empact and are taken from their parent PR #25152.
#### Context
There is an ongoing effort to decouple the `ArgsManager` used for command line parsing user-provided arguments from the libbitcoinkernel libr
...
💬 MarcoFalke commented on pull request "Avoid integer overflow in CheckDiskSpace":
(https://github.com/bitcoin/bitcoin/pull/27235#discussion_r1132256438)
In that case, `additional_bytes_needed` can be clamped by `AssumedBlockchainSize`?
[Or maybe by twice the `AssumedBlockchainSize` to accommodate for an imaginary user that just bought a 1TB drive (intending to use it for a few years) and set pruning to 999GB, but doesn't have that much space available?]
(https://github.com/bitcoin/bitcoin/pull/27235#discussion_r1132256438)
In that case, `additional_bytes_needed` can be clamped by `AssumedBlockchainSize`?
[Or maybe by twice the `AssumedBlockchainSize` to accommodate for an imaginary user that just bought a 1TB drive (intending to use it for a few years) and set pruning to 999GB, but doesn't have that much space available?]
💬 willcl-ark commented on issue "False positive collisions could occur because of GetBucketPosition":
(https://github.com/bitcoin/bitcoin/issues/22554#issuecomment-1463673221)
@janus are you able to reproduce this with the current version of the software? If not perhaps this can be closed.
Reading the codeblock as it stands today, it still appears that the entry will only be added if it does not already exist there:
https://github.com/bitcoin/bitcoin/blob/f7bdcfc83f5753349018be3b5a663c8923d1a5eb/src/addrman.cpp#L605-L627
> Check should be made to confirm if pinfo is empty while checking fInsert.
We will always have `pinfo` at the check, as it's either foun
...
(https://github.com/bitcoin/bitcoin/issues/22554#issuecomment-1463673221)
@janus are you able to reproduce this with the current version of the software? If not perhaps this can be closed.
Reading the codeblock as it stands today, it still appears that the entry will only be added if it does not already exist there:
https://github.com/bitcoin/bitcoin/blob/f7bdcfc83f5753349018be3b5a663c8923d1a5eb/src/addrman.cpp#L605-L627
> Check should be made to confirm if pinfo is empty while checking fInsert.
We will always have `pinfo` at the check, as it's either foun
...
📝 MarcoFalke opened a pull request: " refactor: Consistenly use context args over gArgs in node/interfaces "
(https://github.com/bitcoin/bitcoin/pull/27239)
Currently `node/interfaces.cpp` uses a mix of `gArgs` vs `m_context->args`. This is fine, because outside of tests those should be identical. However, it makes the code inconsistent and harder to use in tests.
Fix that by using `args` from the context consistently. Do the same in `init.cpp`, where `gArgs` and `args` are inconsistently used in the same scope or even line.
(https://github.com/bitcoin/bitcoin/pull/27239)
Currently `node/interfaces.cpp` uses a mix of `gArgs` vs `m_context->args`. This is fine, because outside of tests those should be identical. However, it makes the code inconsistent and harder to use in tests.
Fix that by using `args` from the context consistently. Do the same in `init.cpp`, where `gArgs` and `args` are inconsistently used in the same scope or even line.
💬 MarcoFalke commented on issue "False positive collisions could occur because of GetBucketPosition":
(https://github.com/bitcoin/bitcoin/issues/22554#issuecomment-1463676435)
Closing for now, but happy to reopen if there is more info provided.
(https://github.com/bitcoin/bitcoin/issues/22554#issuecomment-1463676435)
Closing for now, but happy to reopen if there is more info provided.
✅ MarcoFalke closed an issue: "False positive collisions could occur because of GetBucketPosition"
(https://github.com/bitcoin/bitcoin/issues/22554)
(https://github.com/bitcoin/bitcoin/issues/22554)
💬 rebroad commented on pull request "Scale network graph based on time interval":
(https://github.com/bitcoin-core/gui/pull/539#issuecomment-1463676528)
> There hasn't been much activity lately and the patch still needs rebase. What is the status here?
>
> * Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
> * Is it no longer relevant? ➡️ Please close.
> * Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.
How do I label it as up for grabs?
(https://github.com/bitcoin-core/gui/pull/539#issuecomment-1463676528)
> There hasn't been much activity lately and the patch still needs rebase. What is the status here?
>
> * Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
> * Is it no longer relevant? ➡️ Please close.
> * Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.
How do I label it as up for grabs?
💬 hebasto commented on pull request "Scale network graph based on time interval":
(https://github.com/bitcoin-core/gui/pull/539#issuecomment-1463678070)
> How do I label it as up for grabs?
Done.
(https://github.com/bitcoin-core/gui/pull/539#issuecomment-1463678070)
> How do I label it as up for grabs?
Done.
💬 willcl-ark commented on pull request "github: Switch to yaml issue templates":
(https://github.com/bitcoin/bitcoin/pull/27025#discussion_r1132270445)
Hmmm, for me as a non admin this repo now looks like:
The green `View Policy` button takes me to a page which includes "Policy" and "Advisories", defaulting to showing the security policy `SECURITY.md`.
The row below that, "Bitcoin Core Security Policy" with the non-green `Open` button links me straight to `SECURITY.md`, which I agree is in theory somewhat redundant. If there i
...
(https://github.com/bitcoin/bitcoin/pull/27025#discussion_r1132270445)
Hmmm, for me as a non admin this repo now looks like:
The green `View Policy` button takes me to a page which includes "Policy" and "Advisories", defaulting to showing the security policy `SECURITY.md`.
The row below that, "Bitcoin Core Security Policy" with the non-green `Open` button links me straight to `SECURITY.md`, which I agree is in theory somewhat redundant. If there i
...
💬 willcl-ark commented on issue "[RFC] Dealing with RPCs that have a lot of positional options":
(https://github.com/bitcoin/bitcoin/issues/22575#issuecomment-1463691588)
Can this be closed now that we have https://github.com/bitcoin/bitcoin/pull/19762 ? Or are we leaving it open to remember to address [this](https://github.com/bitcoin/bitcoin/issues/22575#issuecomment-889252965) comment:
> I think another important thing our RPC server is missing is the ability to require option-style parameters to be passed by name, and to disallow passing them by position.
(https://github.com/bitcoin/bitcoin/issues/22575#issuecomment-1463691588)
Can this be closed now that we have https://github.com/bitcoin/bitcoin/pull/19762 ? Or are we leaving it open to remember to address [this](https://github.com/bitcoin/bitcoin/issues/22575#issuecomment-889252965) comment:
> I think another important thing our RPC server is missing is the ability to require option-style parameters to be passed by name, and to disallow passing them by position.
💬 MarcoFalke commented on issue "[RFC] Dealing with RPCs that have a lot of positional options":
(https://github.com/bitcoin/bitcoin/issues/22575#issuecomment-1463698107)
> Can this be closed now that we have #19762 ? Or are we leaving it open to remember to address [this](https://github.com/bitcoin/bitcoin/issues/22575#issuecomment-889252965) comment:
>
> > I think another important thing our RPC server is missing is the ability to require option-style parameters to be passed by name, and to disallow passing them by position.
That is implemented in #26485, no?
(https://github.com/bitcoin/bitcoin/issues/22575#issuecomment-1463698107)
> Can this be closed now that we have #19762 ? Or are we leaving it open to remember to address [this](https://github.com/bitcoin/bitcoin/issues/22575#issuecomment-889252965) comment:
>
> > I think another important thing our RPC server is missing is the ability to require option-style parameters to be passed by name, and to disallow passing them by position.
That is implemented in #26485, no?
💬 willcl-ark commented on issue "[RFC] Dealing with RPCs that have a lot of positional options":
(https://github.com/bitcoin/bitcoin/issues/22575#issuecomment-1463707740)
Oh yea, looks like it. Cool!
(https://github.com/bitcoin/bitcoin/issues/22575#issuecomment-1463707740)
Oh yea, looks like it. Cool!
💬 MarcoFalke commented on pull request "assumeutxo: net_processing changes":
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1132286555)
nit (feel free to ignore): What about passing a `const&` ref here? Otherwise someone could pass in a `nullptr` and run into UB at runtime?
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1132286555)
nit (feel free to ignore): What about passing a `const&` ref here? Otherwise someone could pass in a `nullptr` and run into UB at runtime?
💬 MarcoFalke commented on pull request "assumeutxo: net_processing changes":
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1132291848)
nit: No need to call `get` if you call `*` later on. Also, can inline the assert:
```suggestion
return *Assert(m_ibd_chainstate);
```
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1132291848)
nit: No need to call `get` if you call `*` later on. Also, can inline the assert:
```suggestion
return *Assert(m_ibd_chainstate);
```
👍 brunoerg approved a pull request: "Add test and docs for getblockfrompeer with pruning"
(https://github.com/bitcoin/bitcoin/pull/23813)
re-ACK fe329dc936d1e02da406345e4223e11d1fa6fb38
(https://github.com/bitcoin/bitcoin/pull/23813)
re-ACK fe329dc936d1e02da406345e4223e11d1fa6fb38