💬 TheCharlatan commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-2998028456)
Tangentially related to the topic here, have we discussed adding the capnp files to the releases somewhere yet? I'd be interested what the thoughts are around that. I think at least for the mining interface, where some thought has been spent on how to make it usable for an external project this would make sense. The alternative of having to regularly copy them over to the target project seems less ideal to me.
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-2998028456)
Tangentially related to the topic here, have we discussed adding the capnp files to the releases somewhere yet? I'd be interested what the thoughts are around that. I think at least for the mining interface, where some thought has been spent on how to make it usable for an external project this would make sense. The alternative of having to regularly copy them over to the target project seems less ideal to me.
💬 mzumsande commented on pull request "test: allow all functional tests to be run or skipped with --usecli":
(https://github.com/bitcoin/bitcoin/pull/32290#discussion_r2162581057)
Makes sense - I renamed it to `_rpc`, added a comment and fixed the additional spots. There is a legit for `rpc` use in setting `ensure_ascii`.
(https://github.com/bitcoin/bitcoin/pull/32290#discussion_r2162581057)
Makes sense - I renamed it to `_rpc`, added a comment and fixed the additional spots. There is a legit for `rpc` use in setting `ensure_ascii`.
💬 mzumsande commented on pull request "test: allow all functional tests to be run or skipped with --usecli":
(https://github.com/bitcoin/bitcoin/pull/32290#discussion_r2162581154)
Done.
(https://github.com/bitcoin/bitcoin/pull/32290#discussion_r2162581154)
Done.
💬 mzumsande commented on pull request "test: allow all functional tests to be run or skipped with --usecli":
(https://github.com/bitcoin/bitcoin/pull/32290#discussion_r2162582593)
yes - I removed this in commit "test: allow all functional tests to be run or skipped with --usecli" which touches this line anyway.
(https://github.com/bitcoin/bitcoin/pull/32290#discussion_r2162582593)
yes - I removed this in commit "test: allow all functional tests to be run or skipped with --usecli" which touches this line anyway.
🤔 glozow reviewed a pull request: "doc: archive 28.2 release notes"
(https://github.com/bitcoin/bitcoin/pull/32797#pullrequestreview-2951621314)
ACK 907842363c64621b32acbb0d02e8500058b6ae57
via `git diff HEAD:doc/release-notes/release-notes-28.2.md v28.2:doc/release-notes.md`
(https://github.com/bitcoin/bitcoin/pull/32797#pullrequestreview-2951621314)
ACK 907842363c64621b32acbb0d02e8500058b6ae57
via `git diff HEAD:doc/release-notes/release-notes-28.2.md v28.2:doc/release-notes.md`
🚀 glozow merged a pull request: "doc: archive 28.2 release notes"
(https://github.com/bitcoin/bitcoin/pull/32797)
(https://github.com/bitcoin/bitcoin/pull/32797)
💬 mzumsande commented on pull request "test: allow all functional tests to be run or skipped with --usecli":
(https://github.com/bitcoin/bitcoin/pull/32290#issuecomment-2998106087)
[60106f3](https://github.com/bitcoin/bitcoin/commit/60106f32852524498d5d6e958a3bcabdfb06ee1a) to [666016e](https://github.com/bitcoin/bitcoin/commit/666016e56b28b77f798dc85c767b95c1ca0abfae):
Addressed feedback by @maflcko and rebased.
(https://github.com/bitcoin/bitcoin/pull/32290#issuecomment-2998106087)
[60106f3](https://github.com/bitcoin/bitcoin/commit/60106f32852524498d5d6e958a3bcabdfb06ee1a) to [666016e](https://github.com/bitcoin/bitcoin/commit/666016e56b28b77f798dc85c767b95c1ca0abfae):
Addressed feedback by @maflcko and rebased.
🤔 pinheadmz reviewed a pull request: "Introduce SockMan ("lite"): low-level socket handling for HTTP"
(https://github.com/bitcoin/bitcoin/pull/32747#pullrequestreview-2951071498)
Rebase to address review by @vasild THANKS!
(https://github.com/bitcoin/bitcoin/pull/32747#pullrequestreview-2951071498)
Rebase to address review by @vasild THANKS!
💬 pinheadmz commented on pull request "Introduce SockMan ("lite"): low-level socket handling for HTTP":
(https://github.com/bitcoin/bitcoin/pull/32747#discussion_r2162235428)
👍
(https://github.com/bitcoin/bitcoin/pull/32747#discussion_r2162235428)
👍
💬 pinheadmz commented on pull request "Introduce SockMan ("lite"): low-level socket handling for HTTP":
(https://github.com/bitcoin/bitcoin/pull/32747#discussion_r2162325395)
👍
(https://github.com/bitcoin/bitcoin/pull/32747#discussion_r2162325395)
👍
💬 pinheadmz commented on pull request "Introduce SockMan ("lite"): low-level socket handling for HTTP":
(https://github.com/bitcoin/bitcoin/pull/32747#discussion_r2162331716)
👍
(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 ;-)
(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
(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}) {
```
(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
...
(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
(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`
(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
...
(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
(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
...
(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
...