💬 pinheadmz commented on pull request "Introduce SockMan ("lite"): low-level socket handling for HTTP":
(https://github.com/bitcoin/bitcoin/pull/32747#discussion_r2162331716)
👍

💬 pinheadmz commented on pull request "Introduce SockMan ("lite"): low-level socket handling for HTTP":
(https://github.com/bitcoin/bitcoin/pull/32747#discussion_r2162324748)
Good catch, taken. I was hoping to get away with only ever using one client in this test but this makes more sense for coverage anyway ;-)

💬 pinheadmz commented on pull request "Introduce SockMan ("lite"): low-level socket handling for HTTP":
(https://github.com/bitcoin/bitcoin/pull/32747#discussion_r2162288958)
Oh yes thanks, done by inserting a move-only commit

💬 theStack commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2162793069)
micro-optimization nit: as a DoS score of 1 is still okay, could only consider peers which are strictly above?
```suggestion
// Performance optimization: only consider peers with a DoS score > 1.
if (entry.GetDosScore(max_ann, max_mem) >> FeeFrac{1, 1}) {
```

💬 theStack commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2162806157)
to get rid of unrelated fee/size terminology, I wonder if a dedicated dos score type would make sense, like e.g.:
```
// Use FeeFrac class to represent the DoS score as fraction,
// with more generic member methods for (de)nominator access
class DosScore : public FeeFrac {
private:
// prevent direct access of base fraction values
using FeeFrac::fee, FeeFrac::size;
public:
using FeeFrac::FeeFrac; // inherit constructors
int64_t nominator() const { return fee; }
int3
...

💬 theStack commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2162799520)
could add `Assume`s here before the subtractions each (`m_total_usage >= ann.GetUsage()`, `m_count_announcements >= 1`) to ensure they won't underflow

💬 theStack commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2162780473)
should this be
```suggestion
auto it_lower = m_orphans.get<ByWtxid>().lower_bound(ByWtxidView{wtxid, MIN_PEER});
return it_lower != m_orphans.get<ByWtxid>().end() && it_lower->m_tx->GetWitnessHash() == wtxid;
```
instead, if only for consistency? There are two more places I've found where an indexed iterator is compared to a non-indexed `.end()`: `::AddAnnouncer`, `::GetTx`

📝 theStack opened a pull request: "mempool: use `FeeFrac` for ancestor/descendant score comparators"
(https://github.com/bitcoin/bitcoin/pull/32799)
Rather than determining fee-rates for the mempool index scores and comparators manually in a rather tedious way (even involving floating-points), use the `FeeFrac` class [1] to simplify and deduplicate the code. Note that though this is intended to be a refactoring PR, there might be subtle differences in behaviour due to floating-point arithmetic involved in the original code (to avoid overflows at the cost of precision loss), but these shouldn't matter.

[1] introduced in PR #29242, commit c
...

💬 theStack commented on pull request "refactor: CFeeRate encapsulates FeeFrac internally":
(https://github.com/bitcoin/bitcoin/pull/32750#issuecomment-2998558126)
Concept ACK

📝 musaHaruna opened a pull request: "Distinguish between vsize and sigop adjusted mempool vsize"
(https://github.com/bitcoin/bitcoin/pull/32800)
### Motivation and Problem
Fixes [32775](https://github.com/bitcoin/bitcoin/issues/32775)
CTxMemPoolEntry::GetTxSize() returns the larger of two values: the BIP 141 virtual size (vsize) and the "sigop-adjusted size." This sigop-adjusted size is used by mempool validation and mining algorithms as a safeguard to prevent overfilling blocks with transactions that approach both the weight and signature operation (sigop) limits in a way that could exploit block space efficiency.

In the current im
...

💬 Sjors commented on pull request "descriptors: Allow `H` as a hardened indicator":
(https://github.com/bitcoin/bitcoin/pull/32788#issuecomment-2999109559)
> you could encode an xpub into a format like bech32, encoded into QR alphanumeric mode and save ~15% over a base58 encoded xpub in QR binary mode

I think it would be nice in general to have a bech32m encoding for extended keys, because part of the reason why descriptors look weird is the mix of bech32 and base58.

✅ polespinasa closed a pull request: "wallet, rpc: remove settxfee and paytxfee"
(https://github.com/bitcoin/bitcoin/pull/32138)

💬 polespinasa commented on pull request "wallet, rpc: remove settxfee and paytxfee":
(https://github.com/bitcoin/bitcoin/pull/32138#issuecomment-2999139610)
Will re-open closer to 31.0 release

💬 willcl-ark commented on pull request "util: detect and warn when using exFAT on MacOS":
(https://github.com/bitcoin/bitcoin/pull/31453#issuecomment-2999233449)
> No need to mention the warning for the blocksdir since it just a folder in the datadir; It's whats being checked in [ismaelsadeeq@ddb02a6](https://github.com/ismaelsadeeq/bitcoin/commit/ddb02a6beb0ec892c18b43a55d53dc2daf11e555)

Ok, we now check if blocksdir is inside datadir, and removed the long URL from the warning.

💬 maflcko commented on pull request "test: allow all functional tests to be run or skipped with --usecli":
(https://github.com/bitcoin/bitcoin/pull/32290#issuecomment-2999413010)
only change is some nits

re-ACK 666016e56b28b77f798dc85c767b95c1ca0abfae 🔀

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment:
...

👍 dergoegge approved a pull request: "node: cap `-maxmempool` and `-dbcache` values for 32-bit"
(https://github.com/bitcoin/bitcoin/pull/32530#pullrequestreview-2952838460)
Code review ACK 9f8e7b0b3b787b873045a4a8194e77d0b0a2b3b6

The chosen limits seem reasonable to me.

🤔 maflcko reviewed a pull request: "rpc: use CScheduler for HTTPRPCTimer"
(https://github.com/bitcoin/bitcoin/pull/32796#pullrequestreview-2952901140)
lgtm ACK 466722cbe5d6c9ddf35d1590b749d19ec115684e 🎱

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ACK 466722cbe5d6c9dd
...

💬 maflcko commented on pull request "rpc: use CScheduler for HTTPRPCTimer":
(https://github.com/bitcoin/bitcoin/pull/32796#discussion_r2163404769)
```suggestion
explicit HTTPRPCTimerInterface(const std::any& context)
: m_context{*Assert(std::any_cast<NodeContext*>(context))}
```

💬 maflcko commented on pull request "rpc: use CScheduler for HTTPRPCTimer":
(https://github.com/bitcoin/bitcoin/pull/32796#discussion_r2163412918)
if this turns out problematic (intermittently fail), one could also try `mockscheduler`.

💬 maflcko commented on pull request "rpc: use CScheduler for HTTPRPCTimer":
(https://github.com/bitcoin/bitcoin/pull/32796#discussion_r2163408031)
nit: Could probably assert early on construction, rather than when a timer is created, but either is equally fine.