💬 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

💬 achow101 commented on pull request "test: added test to assert TX decode rpc error on submitpackage rpc":
(https://github.com/bitcoin/bitcoin/pull/31139#issuecomment-2452600410)
ACK d7fd766feb2f579bdba0e778bacdeb13103e8282

✅ achow101 closed an issue: "Gracefully handle dropped UPnP support "
(https://github.com/bitcoin-core/gui/issues/843)

🚀 achow101 merged a pull request: "init: warn, don't error, when '-upnp' is set"
(https://github.com/bitcoin/bitcoin/pull/31198)

🚀 achow101 merged a pull request: "test: added test to assert TX decode rpc error on submitpackage rpc"
(https://github.com/bitcoin/bitcoin/pull/31139)

🤔 brunoerg reviewed a pull request: "netinfo: add peer services column and outbound-only option"
(https://github.com/bitcoin/bitcoin/pull/30930#pullrequestreview-2410910457)
crACK 87532fe55856efc063cf81244800da37a015ba75

code looks great, but I didn't test in practice yet.

💬 ismaelsadeeq commented on pull request "Policy: Report reason inputs are non standard":
(https://github.com/bitcoin/bitcoin/pull/29060#discussion_r1826323008)
In this case, a new state is indeed created in `HasNonStandardInput`, but either RVO or an implicit move is performed not a copy?

I think, in general, returning a value is preferred over using out parameters?
same was also recommended here https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c-functions-and-methods

💬 ismaelsadeeq commented on pull request "RPC: Add reserve member function to `UniValue` and use it in `getblock` RPC":
(https://github.com/bitcoin/bitcoin/pull/31179#issuecomment-2452643916)
> Also, the witness stack array inside TxToUniv is a VARR that could be reserved with a parameter resolved at runtime.

Thanks, I've added this in the latest push.