💬 0xB10C commented on pull request "Halt processing of unrequested transactions v2":
(https://github.com/bitcoin/bitcoin/pull/30572#issuecomment-2452328622)
> I was just trying to point out that there might be a bug somewhere - I don't know if so and where exactly and haven't analyzed this in detail but it seemed to me that pre-segwit txns are often considered unrequested, so these might be false positives.

@mzumsande and I had a look at my logs and the ["rate of a few [unsolicited] per minute"](https://github.com/bitcoin/bitcoin/pull/30572#issuecomment-2401651398) I was seeing were mostly non-segwit transactions - not unsolicited transactions. T
...

👍 rkrux approved a pull request: "test: added test to assert TX decode rpc error on submitpackage rpc"
(https://github.com/bitcoin/bitcoin/pull/31139#pullrequestreview-2410623270)
reACK d7fd766feb2f579bdba0e778bacdeb13103e8282

🤔 mzumsande reviewed a pull request: "bench: Remove various extraneous benchmarks"
(https://github.com/bitcoin/bitcoin/pull/31153#pullrequestreview-2410592950)
Concept ACK

I wouldn't oppose deleting some benchmarks, but I also like the idea of using priorities. That, combined with the doc should be sufficient to deter micro-optimiziations in non-critical code.

💬 mzumsande commented on pull request "bench: Remove various extraneous benchmarks":
(https://github.com/bitcoin/bitcoin/pull/31153#discussion_r1826118034)
typo: perforamnce

💬 mzumsande commented on pull request "bench: Remove various extraneous benchmarks":
(https://github.com/bitcoin/bitcoin/pull/31153#discussion_r1826129957)
Not sure I follow this point: Under "denial of service" I would understand attacks that overwhelm the target with repeated requests (which are often but not necessarily identical) and as result may just slow it down / make it unresponsive instead of outright crashing it - while I agree that benchmarks don't really find these, I wouldn't see fuzz tests designed to find crashes as the best way to test these issues (instead of integration tests with some sort of stress)?

💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1826245538)
Resolving since we don't have a cache anymore

💬 jasonribble commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#issuecomment-2452512182)
So C++ noob question. Not sure if this is an appropriate place to ask.

I'm noticing that you're calling `CFeeRate` with an `int`:
https://github.com/bitcoin/bitcoin/blob/280e65d3a7993ea38ba49ae00133a7ba709c193e/src/wallet/test/coinselection_tests.cpp#L20-L38

Okay that's cool, clearly it compiles and runs...but how?

src/policy/feerate.h has the `class` definition. I don't see how it's able to only take an `int`

This is my best bet of how it works

https://github.com/bitcoin/bitcoi
...

💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1826249448)
Agreed the docs for `TxReconciliationTracker::AddToSet` could be improved. I'll update it with a version of your suggestion

💬 sipa commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#issuecomment-2452516136)
@jasonribble `template<std::integral I>` is the same as `template<typename I>`, with the added restriction that `I` must be a type that satisfies the `std::integral` concept. Since `int` does satisfy that concept, `I` can be instantiated as `int`.

💬 ismaelsadeeq commented on pull request "Policy: Report reason inputs are non standard":
(https://github.com/bitcoin/bitcoin/pull/29060#discussion_r1826251662)
Done thanks

💬 ismaelsadeeq commented on pull request "Policy: Report reason inputs are non standard":
(https://github.com/bitcoin/bitcoin/pull/29060#discussion_r1826251925)
Good idea, done

💬 ismaelsadeeq commented on pull request "Policy: Report reason inputs are non standard":
(https://github.com/bitcoin/bitcoin/pull/29060#issuecomment-2452518924)
> > Does bitcoind consider this to be a breaking change? The new error message may be "more accurate", but for clients that were matching on this error to figure out why a transaction was rejected, this breaks old behavior.

I've added a release notes for this

💬 jasonribble commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#issuecomment-2452535019)
> @jasonribble `template<std::integral I>` is the same as `template<typename I>`, with the added restriction that `I` must be a type that satisfies the `std::integral` concept. Since `int` does satisfy that concept, `I` can be instantiated as `int`.

Magical. Thank you 🙏

💬 TheCharlatan commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#issuecomment-2452537469)
> But, I don't see #include <concepts>.

If you check the IWYU logs in the "tidy" CI job you will see that it is indeed missing:
```
[19:28:01.280] /ci_container_base/src/policy/feerate.h should add these lines:
[19:28:01.280] #include <concepts> // for integral
```
This report is generated on each CI run, but the missing includes are not enforced. This can be confusing, so best to always check the CI output.

💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1826266792)
Not too sure about the `return removed == 1`

The assume is there to make sure that if the code happens to be wrong, it is catched on testing. In the current approach, if the code moving data from `delayed_set` to `m_local_set` was broken somehow, this method will return that data was actually removed. If were to return `removed == 1` in the aforementioned case, it would return false, and the caller's logic may break.

I don't think any of these cases is likely to happen given the assume wil
...

💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1826275867)
I just realized this is also the case for the ancestors. Went relaying with ancestors, we should also check that the ancestor we are trying to relay is part of the reconciliation set (i.e. that it has not been previously flooded)

💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1826278558)
This is only called if there are in mempol parents though, the ordering doesn't matter much otherwise

💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#issuecomment-2452563866)
Rebased to make CI green, plus covered (and masked as resolved) some outstanding comments:

https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1818553463
https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1818555798
https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1818577080
https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1818641775
https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1818703437
https://github.com/bitcoin/bitcoin/pull/30116#discussion
...

💬 instagibbs commented on pull request "rpc: Remove submitblock pre-checks":
(https://github.com/bitcoin/bitcoin/pull/31175#issuecomment-2452572531)
my guess is from #10146 this commit ada0caa165905b50db351a56ec124518c922085a looks like a covert fix for `submitblock` with a completely empty block. https://github.com/bitcoin/bitcoin/blob/master/src/rpc/mining.cpp#L1041 shows that it will call:

`UpdateUncommittedBlockStructures -> GetWitnessCommitmentIndex` which would result in UB, before any PoW checks are made.

💬 achow101 commented on pull request "init: warn, don't error, when '-upnp' is set":
(https://github.com/bitcoin/bitcoin/pull/31198#issuecomment-2452594931)
ACK a1b3ccae4be82297fd20f5be15a03eeb477507d0