💬 vasild commented on issue "Remove libevent as a dependency (HTTP / cli / torcontrol)":
(https://github.com/bitcoin/bitcoin/issues/31194#issuecomment-2454744101)
Concept ACK on removing libevent.
In addition to the motivation from the OP, I would add that libevent is non-intuitive and difficult to work with (at least for me, this could be subjective). It was related to a remote crash via RPC: https://github.com/bitcoin/bitcoin/pull/27468
(https://github.com/bitcoin/bitcoin/issues/31194#issuecomment-2454744101)
Concept ACK on removing libevent.
In addition to the motivation from the OP, I would add that libevent is non-intuitive and difficult to work with (at least for me, this could be subjective). It was related to a remote crash via RPC: https://github.com/bitcoin/bitcoin/pull/27468
💬 laanwj commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827750019)
For g++ 13.2 on x86_64 it checks out, this returns 16:
```c++
#include <string>
#include <iostream>
bool data_is_internal(const std::string &s)
{
return s.data() >= reinterpret_cast<const char*>(&s) && s.data() + s.size() - sizeof(s) <= reinterpret_cast<const char*>(&s);
}
int main()
{
std::string s;
while (data_is_internal(s)) {
s += 'a';
}
std::cout << s.size() << std::endl;
return 0;
}
```
But yes it's not possible to guarantee across c++
...
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827750019)
For g++ 13.2 on x86_64 it checks out, this returns 16:
```c++
#include <string>
#include <iostream>
bool data_is_internal(const std::string &s)
{
return s.data() >= reinterpret_cast<const char*>(&s) && s.data() + s.size() - sizeof(s) <= reinterpret_cast<const char*>(&s);
}
int main()
{
std::string s;
while (data_is_internal(s)) {
s += 'a';
}
std::cout << s.size() << std::endl;
return 0;
}
```
But yes it's not possible to guarantee across c++
...
💬 maflcko commented on issue "Intermittent timeout in tsan feature_init.py":
(https://github.com/bitcoin/bitcoin/issues/30586#issuecomment-2454751583)
https://cirrus-ci.com/task/6488439019798528?logs=ci#L3192
(https://github.com/bitcoin/bitcoin/issues/30586#issuecomment-2454751583)
https://cirrus-ci.com/task/6488439019798528?logs=ci#L3192
👍 hodlinator approved a pull request: "cmake: Add `FindQRencode` module and enable `libqrencode` package for MSVC"
(https://github.com/bitcoin/bitcoin/pull/31173#pullrequestreview-2413175649)
re-ACK 6b493e5dca53eeafbe3e10150e7bcdc71d8ffae0
Just variant of suggested doc change applied since prior ACK.
(https://github.com/bitcoin/bitcoin/pull/31173#pullrequestreview-2413175649)
re-ACK 6b493e5dca53eeafbe3e10150e7bcdc71d8ffae0
Just variant of suggested doc change applied since prior ACK.
👍 rkrux approved a pull request: "rpc: add getdescriptoractivity"
(https://github.com/bitcoin/bitcoin/pull/30708#pullrequestreview-2412908865)
tACK f383db76ec3aaa9391509c1d9cca763d11b6fe00
Successful make and functional tests. Left couple nits. I believe this is a good addition as explained in the PR description.
I tested this RPC in `testnet4` and verified the response for the first 3 relevant blocks.
```
➜ bitcoin git:(2024-08-getdescriptoractivity) ✗ bitcoinclitest scanblocks start \
> '["addr(tb1p9nqshur7c07cnt96l7jlfcq92mvkg89yxguqkk4yx79twanzasus2kz7lg)"]'
➜ bitcoin git:(2024-08-getdescriptoractivity) ✗ bitcoinclite
...
(https://github.com/bitcoin/bitcoin/pull/30708#pullrequestreview-2412908865)
tACK f383db76ec3aaa9391509c1d9cca763d11b6fe00
Successful make and functional tests. Left couple nits. I believe this is a good addition as explained in the PR description.
I tested this RPC in `testnet4` and verified the response for the first 3 relevant blocks.
```
➜ bitcoin git:(2024-08-getdescriptoractivity) ✗ bitcoinclitest scanblocks start \
> '["addr(tb1p9nqshur7c07cnt96l7jlfcq92mvkg89yxguqkk4yx79twanzasus2kz7lg)"]'
➜ bitcoin git:(2024-08-getdescriptoractivity) ✗ bitcoinclite
...
💬 rkrux commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1827645682)
Nit: Would be easier on the eyes to have a similar wording as in the spend object above.
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1827645682)
Nit: Would be easier on the eyes to have a similar wording as in the spend object above.
💬 rkrux commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1827637623)
```suggestion
{RPCResult::Type::STR_HEX, "blockhash", "The blockhash that this receive is in. Empty if in mempool"},
```
A receive can also be in mempool as I checked.
```
➜ bitcoin git:(2024-08-getdescriptoractivity) ✗ bitcoinclitest getdescriptoractivity '[]' \
'["addr(tb1qf2xsp2fqyqfauj9elghz6jx7r3fuj07eu2xtgz)"]'
{
"activity": [
{
"type": "receive",
"address": "tb1qf2xsp2fqyqfauj9elghz6jx7r3fuj07eu2xtgz",
"scriptpubkey_hex": "001
...
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1827637623)
```suggestion
{RPCResult::Type::STR_HEX, "blockhash", "The blockhash that this receive is in. Empty if in mempool"},
```
A receive can also be in mempool as I checked.
```
➜ bitcoin git:(2024-08-getdescriptoractivity) ✗ bitcoinclitest getdescriptoractivity '[]' \
'["addr(tb1qf2xsp2fqyqfauj9elghz6jx7r3fuj07eu2xtgz)"]'
{
"activity": [
{
"type": "receive",
"address": "tb1qf2xsp2fqyqfauj9elghz6jx7r3fuj07eu2xtgz",
"scriptpubkey_hex": "001
...
💬 rkrux commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1827683398)
+1 on the different naming here. Took me a second to realise the need for the difference that is based on the usages later.
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1827683398)
+1 on the different naming here. Took me a second to realise the need for the difference that is based on the usages later.
💬 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.