💬 rkrux commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1827750028)
Should the "**Support for Output Descriptors in Bitcoin Core**" section in this doc also be updated?
https://github.com/bitcoin/bitcoin/blob/master/doc/descriptors.md
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1827750028)
Should the "**Support for Output Descriptors in Bitcoin Core**" section in this doc also be updated?
https://github.com/bitcoin/bitcoin/blob/master/doc/descriptors.md
💬 dergoegge commented on pull request "bench: Remove various extraneous benchmarks":
(https://github.com/bitcoin/bitcoin/pull/31153#discussion_r1827798436)
Fuzz testing does not only find crashes, it finds slow running inputs as well. Fuzz engines usually terminate an input's execution if it takes longer than N seconds to run, usually referred to as a "timeout". See #28812 for examples.
Depending on what the harness is testing, a slow input might indicate a DoS issue (e.g. infinite loop, quadratic behavior, ...).
(https://github.com/bitcoin/bitcoin/pull/31153#discussion_r1827798436)
Fuzz testing does not only find crashes, it finds slow running inputs as well. Fuzz engines usually terminate an input's execution if it takes longer than N seconds to run, usually referred to as a "timeout". See #28812 for examples.
Depending on what the harness is testing, a slow input might indicate a DoS issue (e.g. infinite loop, quadratic behavior, ...).
⚠️ naumenkogs opened an issue: "Mempool leak through the eviction policy"
(https://github.com/bitcoin/bitcoin/issues/31211)
A spy can see whether a transaction exists in a target node's mempool before the node INV-announces it (overcoming the trickle protection).
0. Fill all the inbound slots of the target (up to 117 by default), and make sure your connections are not protected from eviction (e.g., delay block delivery to the target, etc).
1. Relay txA to the peer from connection CSpy.
2. Issue more connections to the target (say, 100 more).
3. If CSpy was not evicted while others were, it's likely CSpy was pro
...
(https://github.com/bitcoin/bitcoin/issues/31211)
A spy can see whether a transaction exists in a target node's mempool before the node INV-announces it (overcoming the trickle protection).
0. Fill all the inbound slots of the target (up to 117 by default), and make sure your connections are not protected from eviction (e.g., delay block delivery to the target, etc).
1. Relay txA to the peer from connection CSpy.
2. Issue more connections to the target (say, 100 more).
3. If CSpy was not evicted while others were, it's likely CSpy was pro
...
💬 l0rinc commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827803472)
Alternatively, maybe a test checking this exact scenario could help:
```C++
BOOST_AUTO_TEST_CASE(sso_size_test)
{
CSerializedNetMsg empty_msg;
CSerializedNetMsg msg_with_type;
msg_with_type.m_type = std::string(CMessageHeader::COMMAND_SIZE, 'X');
BOOST_CHECK_EQUAL(sizeof(msg_with_type), sizeof(empty_msg));
}
```
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827803472)
Alternatively, maybe a test checking this exact scenario could help:
```C++
BOOST_AUTO_TEST_CASE(sso_size_test)
{
CSerializedNetMsg empty_msg;
CSerializedNetMsg msg_with_type;
msg_with_type.m_type = std::string(CMessageHeader::COMMAND_SIZE, 'X');
BOOST_CHECK_EQUAL(sizeof(msg_with_type), sizeof(empty_msg));
}
```
📝 hodlinator opened a pull request: "util: Narrow scope of args (-color, -version, -conf, -datadir)"
(https://github.com/bitcoin/bitcoin/pull/31212)
- Document `-color` as only applying to `-getinfo`, to be less confusing for bitcoin-cli users.
- No longer print version information when getting passed `-noversion`.
- Disallow `-noconf` since a config file is required and previously it would cause us to open the parent ".bitcoin" directory (not a file).
- Disallow `-nodatadir` as we cannot run without one. It was previously interpreted as a mix of unset and as a relative path of "0".
Prompted by investigation in https://github.com/bitco
...
(https://github.com/bitcoin/bitcoin/pull/31212)
- Document `-color` as only applying to `-getinfo`, to be less confusing for bitcoin-cli users.
- No longer print version information when getting passed `-noversion`.
- Disallow `-noconf` since a config file is required and previously it would cause us to open the parent ".bitcoin" directory (not a file).
- Disallow `-nodatadir` as we cannot run without one. It was previously interpreted as a mix of unset and as a relative path of "0".
Prompted by investigation in https://github.com/bitco
...
💬 sipa commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827806882)
@l0rinc That doesn't test anything. `sizeof()` is a compile-time property, that just depends on the arguments' type. Since `empty_msg` and `msg_with_type` are the same type, the check will always be true.
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827806882)
@l0rinc That doesn't test anything. `sizeof()` is a compile-time property, that just depends on the arguments' type. Since `empty_msg` and `msg_with_type` are the same type, the check will always be true.
💬 ryanofsky commented on pull request "build: Make G_FUZZING constexpr, require -DBUILD_FOR_FUZZING=ON to fuzz":
(https://github.com/bitcoin/bitcoin/pull/31191#issuecomment-2454859628)
The use-case which I think this breaks and should be supported is the ability to use fuzzing in a normal build instead of a dedicated build.
As an analogy, if you want use a debugger, the best place to use it is in a dedicated debug build, but you should also be able to generate debug symbols and use debuggers with some limitations in release builds. Or if you want to check memory safety, the best way to do it may be to run MSan or valgrind in dedicated builds, but it should also be possible
...
(https://github.com/bitcoin/bitcoin/pull/31191#issuecomment-2454859628)
The use-case which I think this breaks and should be supported is the ability to use fuzzing in a normal build instead of a dedicated build.
As an analogy, if you want use a debugger, the best place to use it is in a dedicated debug build, but you should also be able to generate debug symbols and use debuggers with some limitations in release builds. Or if you want to check memory safety, the best way to do it may be to run MSan or valgrind in dedicated builds, but it should also be possible
...
💬 l0rinc commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827825362)
Lol, that was a stupid mistake, lemme' try again:
```C++
BOOST_AUTO_TEST_CASE(sso_size_test)
{
auto is_sso{[](const std::string& s, const CSerializedNetMsg& msg) {
const auto msg_start = reinterpret_cast<const char*>(&msg);
const auto msg_end = msg_start + sizeof(msg);
return msg_start <= s.data() && s.data() <= msg_end;
}};
const CSerializedNetMsg empty_msg;
BOOST_CHECK(is_sso(empty_msg.m_type, empty_msg));
CSerializedNetMsg msg_with_type;
msg_with_
...
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827825362)
Lol, that was a stupid mistake, lemme' try again:
```C++
BOOST_AUTO_TEST_CASE(sso_size_test)
{
auto is_sso{[](const std::string& s, const CSerializedNetMsg& msg) {
const auto msg_start = reinterpret_cast<const char*>(&msg);
const auto msg_end = msg_start + sizeof(msg);
return msg_start <= s.data() && s.data() <= msg_end;
}};
const CSerializedNetMsg empty_msg;
BOOST_CHECK(is_sso(empty_msg.m_type, empty_msg));
CSerializedNetMsg msg_with_type;
msg_with_
...
✅ knst closed an issue: "guix: failure on Kubuntu 24-10: error: mount: mount "none" on "/tmp/guix-directory.VEMlin": Permission denied"
(https://github.com/bitcoin/bitcoin/issues/31202)
(https://github.com/bitcoin/bitcoin/issues/31202)
💬 knst commented on issue "guix: failure on Kubuntu 24-10: error: mount: mount "none" on "/tmp/guix-directory.VEMlin": Permission denied":
(https://github.com/bitcoin/bitcoin/issues/31202#issuecomment-2454882552)
Removing app-armor and re-installing guix + reboot helped, thanks for quick help!
P.S. I guess I'm moving to Manjaro; a broken AppArmor package out of the box for 2 continuous releases is disappointing.
(https://github.com/bitcoin/bitcoin/issues/31202#issuecomment-2454882552)
Removing app-armor and re-installing guix + reboot helped, thanks for quick help!
P.S. I guess I'm moving to Manjaro; a broken AppArmor package out of the box for 2 continuous releases is disappointing.
💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1827857362)
Oof, this was making it a lot harder to hit the various cases! Fixed!
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1827857362)
Oof, this was making it a lot harder to hit the various cases! Fixed!
💬 l0rinc commented on pull request "benchmark: Improve SipHash_32b accuracy to avoid potential optimization issues":
(https://github.com/bitcoin/bitcoin/pull/30349#issuecomment-2454914258)
> I don't understand why M1 Macs would have more ops after
My understand is that when we've reused the hash as input, it could only run a single instance since each one depended on the previous one. The new way is more predictable and clang often optimizes more aggressively, I guess it unrolls the instances. I can investigate if you think it's important.
> refresh the results anyway after latest push
Update the description, thanks.
(https://github.com/bitcoin/bitcoin/pull/30349#issuecomment-2454914258)
> I don't understand why M1 Macs would have more ops after
My understand is that when we've reused the hash as input, it could only run a single instance since each one depended on the previous one. The new way is more predictable and clang often optimizes more aggressively, I guess it unrolls the instances. I can investigate if you think it's important.
> refresh the results anyway after latest push
Update the description, thanks.
💬 Abdulkbk commented on pull request "Improve lockunspent validation for vout":
(https://github.com/bitcoin/bitcoin/pull/31205#issuecomment-2454922674)
> Every use of `getInt` represents a whole number. This is not special. If you disagree, it would be good to explain and to provide an example. Also, What real-world end-user-facing issue is this trying to solve?
>
> Tend toward NACK for now, unless there is a reason.
I am new to the codebase and learned that every use of `getInt` represents a whole number after you mentioned it.
For the end users facing this issue, one example I can give, those using `bitcoin-cli lockunspent`, may enco
...
(https://github.com/bitcoin/bitcoin/pull/31205#issuecomment-2454922674)
> Every use of `getInt` represents a whole number. This is not special. If you disagree, it would be good to explain and to provide an example. Also, What real-world end-user-facing issue is this trying to solve?
>
> Tend toward NACK for now, unless there is a reason.
I am new to the codebase and learned that every use of `getInt` represents a whole number after you mentioned it.
For the end users facing this issue, one example I can give, those using `bitcoin-cli lockunspent`, may enco
...
💬 dergoegge commented on pull request "bench: Remove various extraneous benchmarks":
(https://github.com/bitcoin/bitcoin/pull/31153#discussion_r1827872630)
Done. Let me know if you're not happy with the wording.
(https://github.com/bitcoin/bitcoin/pull/31153#discussion_r1827872630)
Done. Let me know if you're not happy with the wording.
💬 dergoegge commented on pull request "bench: Remove various extraneous benchmarks":
(https://github.com/bitcoin/bitcoin/pull/31153#discussion_r1827873746)
Thanks, removed the ExpandDescriptor bench as well. Will comment on how I picked the benches soon.
(https://github.com/bitcoin/bitcoin/pull/31153#discussion_r1827873746)
Thanks, removed the ExpandDescriptor bench as well. Will comment on how I picked the benches soon.
📝 marcofleon opened a pull request: "fuzz: Fix difficulty target generation in `p2p_headers_presync`"
(https://github.com/bitcoin/bitcoin/pull/31213)
In the `p2p_headers_presync` fuzz target, this assertion failed:
```
assert(total_work < chainman.MinimumChainWork());
```
The test previously used `ConsumeIntegralInRange` to generate header difficulty targets within a hardcoded range. The fuzzer found specific values in that range that correspond to very low thresholds due to how [`SetCompact`][setcompact-link] works. The total work of a long enough test chain ended up exceeding `MinimumChainWork`.
Fix this by adding a new `ConsumeAri
...
(https://github.com/bitcoin/bitcoin/pull/31213)
In the `p2p_headers_presync` fuzz target, this assertion failed:
```
assert(total_work < chainman.MinimumChainWork());
```
The test previously used `ConsumeIntegralInRange` to generate header difficulty targets within a hardcoded range. The fuzzer found specific values in that range that correspond to very low thresholds due to how [`SetCompact`][setcompact-link] works. The total work of a long enough test chain ended up exceeding `MinimumChainWork`.
Fix this by adding a new `ConsumeAri
...
💬 instagibbs commented on pull request "fuzz: Fix difficulty target generation in `p2p_headers_presync`":
(https://github.com/bitcoin/bitcoin/pull/31213#issuecomment-2454954020)
To be clear, `nBits` is "deterministic" in that the value, given the chain history, is static, right?
(https://github.com/bitcoin/bitcoin/pull/31213#issuecomment-2454954020)
To be clear, `nBits` is "deterministic" in that the value, given the chain history, is static, right?
💬 sipa commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827884684)
@laanwj If you were to actually incorporate that into this PR, be aware you'll need to use `!std::greater{}(a, b)` instead of `a <= b` when comparing unrelated pointers like this (using `<=` for that is technically unspecified behavior, though it works fine in every real compiler IIRC).
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827884684)
@laanwj If you were to actually incorporate that into this PR, be aware you'll need to use `!std::greater{}(a, b)` instead of `a <= b` when comparing unrelated pointers like this (using `<=` for that is technically unspecified behavior, though it works fine in every real compiler IIRC).
💬 maflcko commented on pull request "Improve lockunspent validation for vout":
(https://github.com/bitcoin/bitcoin/pull/31205#issuecomment-2454965521)
> For the end users facing this issue, one example I can give, those using `bitcoin-cli lockunspent`, may encounter confusion when they un/intentionally pass a floating-point vout and receive the error "JSON integer out of range."
If the error message is confusing, it would be a reason to improve the error message. However, this applies to all RPCs and changing a single RPC seems inconsistent and confusing.
(https://github.com/bitcoin/bitcoin/pull/31205#issuecomment-2454965521)
> For the end users facing this issue, one example I can give, those using `bitcoin-cli lockunspent`, may encounter confusion when they un/intentionally pass a floating-point vout and receive the error "JSON integer out of range."
If the error message is confusing, it would be a reason to improve the error message. However, this applies to all RPCs and changing a single RPC seems inconsistent and confusing.
💬 dergoegge commented on pull request "bench: Remove various extraneous benchmarks":
(https://github.com/bitcoin/bitcoin/pull/31153#issuecomment-2454986180)
I'm not sure if using the priority levels is a good alternative to deleting the benches. Afaict, the priority level was introduced to avoid running slow benchmarks in the CI as apposed to indicating how "important" they are. As a result, we're not catching issues with these tests until someone executes them (e.g. https://github.com/bitcoin/bitcoin/issues/29061#issuecomment-1852140217) and I'd prefer not to expand on that.
(https://github.com/bitcoin/bitcoin/pull/31153#issuecomment-2454986180)
I'm not sure if using the priority levels is a good alternative to deleting the benches. Afaict, the priority level was introduced to avoid running slow benchmarks in the CI as apposed to indicating how "important" they are. As a result, we're not catching issues with these tests until someone executes them (e.g. https://github.com/bitcoin/bitcoin/issues/29061#issuecomment-1852140217) and I'd prefer not to expand on that.