💬 laanwj commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827722013)
It's the total memory, making the assumption about the underlying implementation of std::string. Could add that here too, sure.

💬 l0rinc commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827724311)
Don't need to duplicate it, just want to make sure I understand this exceptional case.

💬 laanwj commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827725377)
It's still useful for one purpose: to count bytes going over the network for bandwidth monitoring. This icnludes the statistics per message type.

💬 l0rinc commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827726883)
Got it, thanks for clarifying!

💬 sipa commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827728402)
Message type strings are at most 12 bytes long (that's the limit in both the v1 and v2 transport protocol), while typical standard library implementation allow this optimization up to 15 bytes.

We could actually test for it exactly though: add a `DynamicUsage(const std::string& s)` function which checks if `s.data() >= &s && s.data() + s.size() - sizeof(s) <= &s`, and if so, return 0.

💬 ryanofsky commented on pull request "kernel, refactor: return error status on all fatal errors":
(https://github.com/bitcoin/bitcoin/pull/29700#issuecomment-2454719003)
Rebased 3ab9a46ac67964c9d5ad54120b680b5eee7f16f0 -> 05438605ba95df14705bf2f57924455f3b2e40e1 ([`pr/fatalresult.21`](https://github.com/ryanofsky/bitcoin/commits/pr/fatalresult.21) -> [`pr/fatalresult.22`](https://github.com/ryanofsky/bitcoin/commits/pr/fatalresult.22), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/fatalresult.21-rebase..pr/fatalresult.22)) due to various conflicts. Still have not moved InterruptResult from success type to failure type yet (https://github.com/bitcoin/
...

💬 ryanofsky commented on pull request "Add util::ResultPtr class":
(https://github.com/bitcoin/bitcoin/pull/26022#issuecomment-2454732698)
Rebased 5f6f91c5fd848fecdc71db25ce1c5ddbedc78f29 -> c7e73097f4a8336e23d098c2fb4296319aba318e ([`pr/bresult-ptr.8`](https://github.com/ryanofsky/bitcoin/commits/pr/bresult-ptr.8) -> [`pr/bresult-ptr.9`](https://github.com/ryanofsky/bitcoin/commits/pr/bresult-ptr.9), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/bresult-ptr.8-rebase..pr/bresult-ptr.9)) on top of updated base PR (no changes or conflicts)

💬 ryanofsky commented on pull request "refactor: Use util::Result class for wallet loading":
(https://github.com/bitcoin/bitcoin/pull/25722#issuecomment-2454742154)
Rebased 2b0e70295ed6ff4be91cc5775ddaa9351a1ed430 -> caed6aec4241a2e5d849c9446725c889348e1b7e ([`pr/bresult-load.30`](https://github.com/ryanofsky/bitcoin/commits/pr/bresult-load.30) -> [`pr/bresult-load.31`](https://github.com/ryanofsky/bitcoin/commits/pr/bresult-load.31), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/bresult-load.30-rebase..pr/bresult-load.31)) due to various conflicts

💬 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

💬 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++
...

💬 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

👍 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.

👍 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
...

💬 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.

💬 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
...

💬 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.

💬 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

💬 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, ...).

⚠️ 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
...

💬 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));
}
```