💬 achow101 commented on pull request "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip":
(https://github.com/bitcoin/bitcoin/pull/30111#issuecomment-2174342443)
ACK 0e0c422aedd4009ab34eca127e4904d15e81f5be

Not as familiar with locking in their area, so not super confident on the `cs_main` related changes, but the rest (removing other mutexes and consolidating under `m_tx_download_mutex`) looks fine.

💬 rkrux commented on pull request "test: Added test coverage to listsinceblock rpc":
(https://github.com/bitcoin/bitcoin/pull/30195#discussion_r1643385636)
Understood now!
Thanks @achow101, this is very helpful.

💬 AngusP commented on pull request "test: Add Compact Block Encoding test `ReceiveWithExtraTransactions` covering non-empty `extra_txn`":
(https://github.com/bitcoin/bitcoin/pull/30237#discussion_r1643388352)
I think that should be OK, assuming it doesn't happen to hit a case where there's a short-ID collision

💬 AngusP commented on pull request "test: Add Compact Block Encoding test `ReceiveWithExtraTransactions` covering non-empty `extra_txn`":
(https://github.com/bitcoin/bitcoin/pull/30237#discussion_r1643388594)
Sure, seems sensible to me

👍 rkrux approved a pull request: "test: fix MiniWallet script-path spend (missing parity bit in leaf version)"
(https://github.com/bitcoin/bitcoin/pull/30076#pullrequestreview-2123842586)
reACK [e4b0dab](https://github.com/bitcoin/bitcoin/pull/30076/commits/e4b0dabb2115dc74e9c5794ddca3822cd8301c72)

Applied the shared patch again and the following tests fail on master with the mentioned error. Though I've not been able to go through the corresponding control block C++ code yet :(

```
raise JSONRPCException(response['error'], status)
test_framework.authproxy.JSONRPCException: mandatory-script-verify-flag-failed (Witness program hash mismatch) (-26)
```

```
mempool_limi
...

💬 rkrux commented on pull request "test: fix MiniWallet script-path spend (missing parity bit in leaf version)":
(https://github.com/bitcoin/bitcoin/pull/30076#discussion_r1643398546)
Is the sole reason for creating 10 tagged wallets to be thorough? Or is there any other reason as well?

💬 rkrux commented on pull request "test: fix MiniWallet script-path spend (missing parity bit in leaf version)":
(https://github.com/bitcoin/bitcoin/pull/30076#discussion_r1643399436)
i.e. to target the following assumption
> we were so far just lucky that the used combinations of relevant data (internal pubkey, leaf script / version) didn't result in a tweaked pubkey with odd y-parity.

💬 achow101 commented on pull request "Encapsulate warnings in generalized node::Warnings and remove globals":
(https://github.com/bitcoin/bitcoin/pull/30058#issuecomment-2174371181)
ACK 260f8da71a35232d859d7705861fc1a88bfbbe81

💬 rkrux commented on pull request "test: Validate oversized transactions or without inputs":
(https://github.com/bitcoin/bitcoin/pull/29862#discussion_r1643416859)
Nice to have: To reduce duplication by using lambda expressions and captures.

👍 rkrux approved a pull request: "test: Validate oversized transactions or without inputs"
(https://github.com/bitcoin/bitcoin/pull/29862#pullrequestreview-2123875126)
reACK [fa9e680](https://github.com/bitcoin/bitcoin/pull/29862/commits/fa9e680b0ca3c401a33e9901e8732128113be85e)

Re-tested by running make and make check, both successful.
Mentioned couple suggestions but not blockers from my side, upto author.

💬 rkrux commented on pull request "test: Validate oversized transactions or without inputs":
(https://github.com/bitcoin/bitcoin/pull/29862#discussion_r1643413535)
Would be nice to add a comment explaining the presence of this constant 70 for future reference.

💬 prusnak commented on pull request "Revert "contrib: macdeploy: monkey-patch gen-sdk to be deterministic"":
(https://github.com/bitcoin/bitcoin/pull/30282#issuecomment-2174418333)
utACK

👍 prusnak approved a pull request: "Revert "contrib: macdeploy: monkey-patch gen-sdk to be deterministic""
(https://github.com/bitcoin/bitcoin/pull/30282#pullrequestreview-2123899637)
utACK

💬 achow101 commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#issuecomment-2174428338)
ACK 94ed4fbf8e1a396c650b5134d396d6c0be35ce10

🚀 achow101 merged a pull request: "Encapsulate warnings in generalized node::Warnings and remove globals"
(https://github.com/bitcoin/bitcoin/pull/30058)

💬 achow101 commented on pull request "Release `LockData::dd_mutex` before calling `*_detected` functions":
(https://github.com/bitcoin/bitcoin/pull/26781#issuecomment-2174448016)
@hebasto Are you planning on addressing review feedback here?

***

> I think it would be better if it had another commit getting rid of StdMutex/StdLockGuard. Otherwise nothing is really taking advantage of the change.

I agree. The PR description sounded like this was going to be the ultimate change, but it does not appear to have actually been done.

🚀 achow101 merged a pull request: "Cluster size 2 package rbf"
(https://github.com/bitcoin/bitcoin/pull/28984)

💬 ishaanam commented on pull request "wallet, rpc: add anti-fee-sniping to `send` and `sendall`":
(https://github.com/bitcoin/bitcoin/pull/28944#issuecomment-2174450450)
> > > ```diff
> > > --- a/test/functional/wallet_import_rescan.py
> > > +++ b/test/functional/wallet_import_rescan.py
> > > @@ -293,6 +293,7 @@ class ImportRescanTest(BitcoinTestFramework):
> > > add_to_wallet=False,
> > > inputs=[unspent_txid_map[variant.initial_txid]],
> > > outputs=[{ADDRESS_BCRT1_UNSPENDABLE : variant.initial_amount}],
> > > + locktime=0,
> > > subtract_fee_from_outputs=[0]
> > >
...

💬 cbergqvist commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1643503903)
(Came across this block again while working on something else. It struck me that the exception handling looked redundant as we catch the same exact exception types inside `JSONRPCExec(jreq, /*catch_errors=*/true`). *But* the `jreq.parse()` call will throw on missing methods etc.

One could move out `response = JSONRPCExec(jreq, /*catch_errors=*/true);` to after the block, but then one would need to guard against using a `jreq` that failed to parse. So the current version of the block is probab
...

💬 mzumsande commented on issue "RFC: Assumeutxo and large forks and reorgs":
(https://github.com/bitcoin/bitcoin/issues/30288#issuecomment-2174570099)
The problem I see with allowing the background sync to target the most-work chain is that it doesn't seem to fit well into the design philosophy that has been merged so far. The background sync is treated as a low-priority task that is mostly done as an extra precaution after having synced the snapshot to the tip. At some point in the future (I know this would be contentious) we might even want to give users the opt-in option to skip it completely to save bandwidth.
But as it is, the background
...

📝 CharlesCNorton opened a pull request: "fix: typo in benchmark documentation"
(https://github.com/bitcoin/bitcoin/pull/30296)
Corrected "coins selection" to "coin selection" for consistency with the other items listed.

Justification:

Consistency: Aligns with other singular nouns in the list.

<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation f
...