💬 maflcko commented on pull request "Improve lockunspent validation for vout":
(https://github.com/bitcoin/bitcoin/pull/31205#issuecomment-2454689466)
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.

💬 laanwj commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827719079)
Right, it can, when `reserve()` is called with a larger amount, or when the vector is shortened. But it does represent the actually-allocated size which seems what we want here.

💬 l0rinc commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827719853)
Can we measure whether this is indeed the case? What is the reason for `assuming it fits in the "small string" optimization area`?

💬 laanwj commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827720254)
Correct, it's computes the dynamic usage only.

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