💬 l0rinc commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2614428365)
This may be useful during development, but it's basically testing the testing logic itself, isn't it? If you keep them, can you add them after the initializations to have separate init and check sections, something like:
```C++
const int start_height{(chain_size - 1) - ctx.randrange(chain_size / 2)}; // pick a height in the second half of the chain
const int delta{min_distance + ctx.randrange(start_height - min_distance + 1)}; // pick a target height, earlier by at least min_distance
const
...

💬 l0rinc commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2614408109)
I think you mean that the second and third **set** bits are zerod, right?
```C++
// Even heights: clear the lowest set bit.
...
// Odd heights: clear the 2nd and 3rd set bits.
```

💬 l0rinc commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2614473206)
Given that we've already called `BuildSkip`, do we need to test this directly or can we do it indirectly as well?

```suggestion
const int height_skip{pindexWalk->pskip->nHeight};
const int height_skip_prev{pindexWalk->pprev->pskip->nHeight};
```

💬 Sjors commented on pull request "doc: update interface, --stdin flag, `signtx` (#31005)":
(https://github.com/bitcoin/bitcoin/pull/33765#issuecomment-3647551260)
@cobratbq there's certainly no need to sign commits, but when people do sign, it's nice that we don't break the signature.

💬 Sjors commented on pull request "doc: improvements to doc/descriptors.md":
(https://github.com/bitcoin/bitcoin/pull/33986#discussion_r2615124857)
Looks like this one only has a markdown version, link gives a 404.

💬 andrewtoth commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2615178895)
> The problem is that in the rare case where the connection does not conclude with `PING`

I don't think this is a rare case at all. It happens frequently where a connection is aborted early e.g. during handshake. In these cases we won't have a tx either, so the proposed solution would not open a new connection either and make private broadcast be less reliable.

I think we should leave this as is. The race condition that is described here is actually the rare occurrence. I did not experienc
...

💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-3647697081)
> `PR` performs better with `100 mb` dbcache than `master` with `7 GB`;

wat

💬 pablomartin4btc commented on pull request "cli: rework -addrinfo cli to use addresses which aren’t filtered for quality/recency":
(https://github.com/bitcoin/bitcoin/pull/26988#discussion_r2615258470)
tiny nit (not a blocker):
```suggestion
getaddrmaninfo RPC internally. Users querying older, unmaintained node versions would need to use an older bitcoin-cli version. (#26988)
```

🤔 pablomartin4btc reviewed a pull request: "cli: rework -addrinfo cli to use addresses which aren’t filtered for quality/recency"
(https://github.com/bitcoin/bitcoin/pull/26988#pullrequestreview-3573191027)
ACK 325f98bca07c3a0c48a46dc18f67376d44c5341d

_(Left a small [suggestion](https://github.com/bitcoin/bitcoin/pull/26988/files#r2615258470) since the older node may be operated by someone other than the CLI user, and upgrading it might not be possible or appropriate when this change is the only reason. I think this would also align with @jonatack’s earlier [note](https://github.com/bitcoin/bitcoin/pull/26988#issuecomment-3559469161).)_

💬 Raimo33 commented on pull request "streams: replace `std::find` with `memchr` (5x improvement)":
(https://github.com/bitcoin/bitcoin/pull/34044#issuecomment-3647753086)
> I would be curious whether a `-reindex -stopafterblockimport` speedup is measurable - since my understanding is that the magic is usually at the beginning anyway, so it's also possible this ends up slowing down the average case.

I've ran a reindex benchmark, see updated PR description. I'm not sure if ~1.2% is an irrelevant gain, but I argue a 5-6x improvement on the `FindByte` method is significant and should be considered for merging regardless, even for possible future use of this method
...

💬 pablomartin4btc commented on pull request "log: Remove brittle and confusing LogPrintLevel":
(https://github.com/bitcoin/bitcoin/pull/34051#discussion_r2615354482)
nit (if you have to retouch and agree):
```suggestion
#define detail_LogIfCategoryAcceptsLevel(category, level, ...) \
```
or `detail_LogIfCategoryIsAcceptedAtLevel`... perhaps it's clearer and more consistent with existing internal call API `LogAcceptCategory()`.

🤔 pablomartin4btc reviewed a pull request: "log: Remove brittle and confusing LogPrintLevel"
(https://github.com/bitcoin/bitcoin/pull/34051#pullrequestreview-3573326964)
ACK fa33a9f5be1beabfee6c217d133cc51dea3b9a63

I agree with the benefits mentioned in the PR description.

⚠️ husstege-e opened an issue: "Wallet: gettransaction(txid).details changes after confirmation of RBF replacement"
(https://github.com/bitcoin/bitcoin/issues/34064)
### Is there an existing issue for this?

- [x] I have searched the existing issues

### Current behaviour

For a replaced transaction (txA), the output of gettransaction(txA, true).details
changes after the replacement transaction (txB) is confirmed.

Specifically, a 'receive' entry present after bumpfee disappears once txB is
confirmed, even though the queried txid remains txA.

After confirmation, repeated calls are stable.


### Expected behaviour

For a given txid, gettransaction(txid).deta
...

🤔 pablomartin4btc reviewed a pull request: "lint: Remove confusing, redundant, and brittle lint-spelling"
(https://github.com/bitcoin/bitcoin/pull/34053#pullrequestreview-3573513114)
ACK fa904fc683c0892eb800ddb986fdc0c646721077

💬 mzumsande commented on pull request "fuzz: Add fuzz target for block index tree and related validation events":
(https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2615504333)
Yes, that's also my understanding. I'm working on a PR that will resolve this.

💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2615528552)
If I understand it correctly it's just wasted work in rare cases - in which case I agree we should tackle it in a follow-up.

🤔 pinheadmz reviewed a pull request: "Broadcast own transactions only via short-lived Tor or I2P connections"
(https://github.com/bitcoin/bitcoin/pull/29415#pullrequestreview-3573153184)
code-review ACK up to a57ef7ddbacebf23c4c50c3fc58615a58339dcc2 (about 2/3rds through). Also built on macos/arm64 and ran unit and functional tests.

Added a few non blocking nits.

Will finish review next week, re-test in warnet, and play with the feature in the GUI ;-)

💬 pinheadmz commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2615489907)
a57ef7ddbacebf23c4c50c3fc58615a58339dcc2

nit: suggestion
`- Mark that given recipient node has confirmed receipt of a transaction`

💬 pinheadmz commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2615289286)
4c8dfd4f7d60c9fb01d7613945f46d453c808237

If we get to this point, the log message will be re-printed fairly rapidly until an OK address is found and resets the counter?

💬 pinheadmz commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2615351368)
...although, we check `IsRoutable()` when adding to AddrMan so I think it's more likely this code never executes. (Uncovered by tests, too: https://corecheck.dev/bitcoin/bitcoin/pulls/29415)

💬 pinheadmz commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2615234534)
4c8dfd4f7d60c9fb01d7613945f46d453c808237

nit: should this be `[in,out]`?