💬 romanz commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2052027205)
Can we return a `string_view` to the internal buffer?
(preventing allocation & copy)

💬 romanz commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2052025637)
Can we return a `string_view` to the internal buffer?
(preventing allocation & copy)

💬 romanz commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2052024440)
Maybe it would be better (performance-wise) to create the string after the loop is over?
```c++
std::optional<std::string> LineReader::ReadLine()
{
if (it == end) {
return std::nullopt;
}

auto line_start = it;
size_t count = 0;
while (it != end) {
char c = static_cast<char>(*it);
++it;
++count;
if (c == '\n') break;
if (count >= max_read) throw std::runtime_error("max_read exceeded by LineReader");
}
co
...

💬 romanz commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2051998964)
nit: maybe using `std::span` will be simpler?
For example:
```c++
auto span = std::as_bytes(std::span(str));
return {span.begin(), span.end()};
```

👍 rkrux approved a pull request: "rpc: Allow fullrbf fee bump in (psbt)bumpfee"
(https://github.com/bitcoin/bitcoin/pull/31953#pullrequestreview-2780807756)
reACK fa86190e6ed2aeda7bcceaf96f52403816bcd751

```
git range-diff fa16051...fa86190
```

New changes: Updating the commit message, the release docs, functional test name & logs, and unit test comment.

💬 rkrux commented on pull request "rpc: Allow fullrbf fee bump in (psbt)bumpfee":
(https://github.com/bitcoin/bitcoin/pull/31953#discussion_r2052122275)
Nit: I missed this in my previous review, but now that the error is not being asserted, this particular test doesn't have any assertions, and understanding it relies on few internals such as the `replaceable` argument not being passed in `bumpfee` RPC and the transaction created on the `peer_node` has `walletrbf=0` set, which ultimately leads to BIP 125 replaceability signalling not being set.
It'd be nice to assert for the transaction inputs sequence by using a more generalised version of the
...

👍 rkrux approved a pull request: "wallet: Automatically repair corrupted metadata with doubled derivation path"
(https://github.com/bitcoin/bitcoin/pull/29124#pullrequestreview-2780893803)
reACK 670d02bc380c22fa4c985b72e8209f475ed0e7d0

```
git range-diff bdcd435...670d02b
```

Using `find` instead of `at` now, few other functional test naming & comments changes done as suggested earlier.

💬 Sjors commented on pull request "test: test MAX_SCRIPT_SIZE for block validity":
(https://github.com/bitcoin/bitcoin/pull/32304#issuecomment-2818160927)
@instagibbs while writing my [reply on a mailinglist post](https://groups.google.com/g/bitcoindev/c/d6ZO7gXGYbQ/m/J0eUMnVdDAAJ) I found myself wondering if the `MAX_SCRIPT_SIZE` limit applied to the text after `OP_RETURN`, either in standardness (if the 83 byte limit is dropped) or in consensus.

An `OP_RETURN` with a bit over 10,000 bytes (but less than 100K) would answer that, without reading the code.

💬 maflcko commented on pull request "test: add test for decoding PSBT with MuSig2 PSBT key types (BIP 373)":
(https://github.com/bitcoin/bitcoin/pull/32305#issuecomment-2818284555)
(fresh CI)

✅ maflcko closed a pull request: "test: add test for decoding PSBT with MuSig2 PSBT key types (BIP 373)"
(https://github.com/bitcoin/bitcoin/pull/32305)

📝 maflcko reopened a pull request: "test: add test for decoding PSBT with MuSig2 PSBT key types (BIP 373)"
(https://github.com/bitcoin/bitcoin/pull/32305)
This PR is a follow-up to #31247 (see https://github.com/bitcoin/bitcoin/pull/31247#pullrequestreview-2427834909) and adds a functional test for decoding PSBTs (using the `decodepsbt` RPC) with MuSig2 per-input and per-output types. The first commit adds the new MuSig2 key types to the test frameworks and extends the PSBT serialization to cope with lists of bytestrings.

💬 maflcko commented on pull request "bitcoin-cli: Add -ipcconnect option":
(https://github.com/bitcoin/bitcoin/pull/32297#issuecomment-2818291536)
Looks like those are real typos that should be fixed:

> ### LLM Linter (✨ experimental)
>
> Possible typos and grammar issues:
>
> The following typos/grammar issues were found in added lines:
>
> 1. Extra “unix’” in the –ipcconnect help text
> Replace
> “…node.sock unix’ but fall back to TCP…”
> with
> “…node.sock’ but fall back to TCP…”
>
> 2. Double negative in JSONErrorReply comment
> Replace
> “…when a request cannot not
...

💬 maflcko commented on pull request "bench: close wallets after migration":
(https://github.com/bitcoin/bitcoin/pull/32309#discussion_r2052293562)
q in e0c95cc123351681ab4326adb746debba33129f0: What are the exact steps to reproduce this "dangling-pointer"? Also, what is the explanation where it is destructed early and where it is used after free?

👍 rkrux approved a pull request: "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`"
(https://github.com/bitcoin/bitcoin/pull/31243#pullrequestreview-2781144870)
ACK acee5c5

> Was that an ACK an you just forgot the word or did you just want to say you reviewed? :)

I wanted to convey I code reviewed & almost ack. The changes made sense to me intuitively but there was one thing that I had not completely reasoned about, so was hesitant in acking earlier: https://github.com/bitcoin/bitcoin/pull/31243#discussion_r2047087551

💬 rkrux commented on pull request "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`":
(https://github.com/bitcoin/bitcoin/pull/31243#discussion_r2052327089)
I see, they were being added for `pkh`, `wpkh`, `combo`, `tr`, `miniscript` ones in `MakeScript` but not for `multisig` & the like. I debugged this flow in on master while importing a `multisig` descriptor and found it quite weird that the origins of all the `pubkeys` involved in the multisig were being added in the `out.origins` inside `ExpandHelper` but not all the `pubkeys` were added in `out.pubkeys`!
Also, `pubkeys` being set inside `MakeScript` but `origins` being set in `ExpandHelper` ma
...

💬 rkrux commented on pull request "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`":
(https://github.com/bitcoin/bitcoin/pull/31243#discussion_r2052331986)
Probably in some follow up: The documentation of the `MakeScripts` function in `DescriptorImpl` class can be updated to get rid of the following line because the origin info is added in the `GetPubKey` functions now and was added in the `ExpandHelper` function previously.

```
The origin info of the provided pubkeys is automatically added.
```

⚠️ Aria-saeid opened an issue: "Double Spending and Its Impact on Digital Security"
(https://github.com/bitcoin/bitcoin/issues/32316)
### Is there an existing issue for this?

- [x] I have searched the existing issues

### Current behaviour

"Tell us what’s wrong" (مشکل چیست؟): 🔹 Double spending is a security threat in digital currencies where a unit of Bitcoin is spent more than once. To prevent this issue, it is proposed to convert even-numbered values to binary, improving transaction security and blockchain integrity.Currently, double spending can occur in certain digital currency transactions, potentially compromising sec
...

✅ willcl-ark closed an issue: "Double Spending and Its Impact on Digital Security"
(https://github.com/bitcoin/bitcoin/issues/32316)

💬 TheCharlatan commented on pull request "test: test MAX_SCRIPT_SIZE for block validity":
(https://github.com/bitcoin/bitcoin/pull/32304#discussion_r2052325094)
I think it would be good to mention that this split is done for pushdata accounting and to use `MAX_SCRIPT_ELEMENT_SIZE` instead of 520. Maybe something like
```
# (OP_PUSHDATA2 + 2 byte length + b'\x01' * 520) * 19 + (1 byte length + b'\x01' * 62)
```

💬 darosior commented on pull request "test: adds coverage to src/validation for invalid tx coinbase":
(https://github.com/bitcoin/bitcoin/pull/32253#issuecomment-2818354053)
I don't think there is any use in keeping this open? The test is strictly redundant, as Marco demonstrated it's already covered both in unit and functional tests.