💬 ariard commented on pull request "Halt processing of unrequested transactions v2":
(https://github.com/bitcoin/bitcoin/pull/30572#issuecomment-2332788365)
@mzumsande

> I don't get this at all.
> The suggestion above was, if you receive an unrequested TX, and you already have it, ignore it but don't disconnect the
> peer. Which is the same as current behavior on master.

Let’s say you have Peer1 and Peer2 announcing the Transaction ABC, if ABC is concurrently fetch from both Peer1 and Peer2, the `GETDATA` (32-bytes) for ABC would have been wasted as outgoing bandwidth by Local Node. This 32 byte leak can be amplified if batch of transactio
...

📝 ismaelsadeeq opened a pull request: "interfaces: #30697 follow ups"
(https://github.com/bitcoin/bitcoin/pull/30828)

This PR addresses the remaining review comments from #30697

1. Disallowed overwriting settings values with a `null` value.
2. Uniformly used the `SettingsAction` enum in all settings methods instead of a boolean parameter.
3. Updated `overwriteRwSetting` to receive the `common::SettingsValue` parameter by value, enabling it to be moved safely.
4. Removed wallet context from the `write_wallet_settings_concurrently` unit test, as it is not needed.

💬 ismaelsadeeq commented on pull request "Bugfix: Ensure Atomicity in Wallet Settings Updates from Chain Interface":
(https://github.com/bitcoin/bitcoin/pull/30697#discussion_r1746265728)
Taken your suggestion @ryanofsky this is fixed in #30828

💬 ismaelsadeeq commented on pull request "Bugfix: Ensure Atomicity in Wallet Settings Updates from Chain Interface":
(https://github.com/bitcoin/bitcoin/pull/30697#discussion_r1746267705)
I've tested this locally by passing an unknown settings name this does not throw.

💬 ismaelsadeeq commented on pull request "Bugfix: Ensure Atomicity in Wallet Settings Updates from Chain Interface":
(https://github.com/bitcoin/bitcoin/pull/30697#discussion_r1746269227)
Taken in #30828

💬 ismaelsadeeq commented on pull request "Bugfix: Ensure Atomicity in Wallet Settings Updates from Chain Interface":
(https://github.com/bitcoin/bitcoin/pull/30697#discussion_r1746269588)
Removed in #30828

💬 ismaelsadeeq commented on pull request "Bugfix: Ensure Atomicity in Wallet Settings Updates from Chain Interface":
(https://github.com/bitcoin/bitcoin/pull/30697#discussion_r1746271611)
Agreed, I see no reason for allowing a null value.
Removed in #30828

💬 ismaelsadeeq commented on pull request "Bugfix: Ensure Atomicity in Wallet Settings Updates from Chain Interface":
(https://github.com/bitcoin/bitcoin/pull/30697#discussion_r1746271896)
Done in #30828

💬 ismaelsadeeq commented on pull request "Bugfix: Ensure Atomicity in Wallet Settings Updates from Chain Interface":
(https://github.com/bitcoin/bitcoin/pull/30697#issuecomment-2332810776)
Thanks for your reviews, @stickies-v and @ryanofsky.
I've addressed the review comments in #30828.


> Thanks. I think a natural cleanup or follow-up to this PR could involve moving the `interfaces::Node` settings methods (`isSettingIgnored`, `getPersistentSetting`, `updateRwSetting`, `forceSetting`, and `resetSettings`) and the `interfaces::Chain` settings methods (`getSetting`, `getSettingList`, `getRwSetting`, `updateRwSetting`, `overwriteRwSetting`, `deleteRwSettings`) into a new `inte
...

🤔 ismaelsadeeq reviewed a pull request: "init: fix init fatal error on invalid negated option value"
(https://github.com/bitcoin/bitcoin/pull/30684#pullrequestreview-2284265281)
Concept ACK
will review

🤔 ajtowns reviewed a pull request: "signet: fixing mining for OP_TRUE challenge"
(https://github.com/bitcoin/bitcoin/pull/29032#pullrequestreview-2284282682)
Concept ACK

💬 ajtowns commented on pull request "signet: fixing mining for OP_TRUE challenge":
(https://github.com/bitcoin/bitcoin/pull/29032#discussion_r1746298241)
Might be better as something like:

```python
signet_spk = tmpl["signet_challenge"]
if trivial_challenge(signet_spk):
signet_solution = None:
else:
...
```

and

```
def trivial_challenge(spkhex):
"""BIP325 allows ..."""
if spkhex == "51":
return True
return False
```

💬 ajtowns commented on pull request "signet: fixing mining for OP_TRUE challenge":
(https://github.com/bitcoin/bitcoin/pull/29032#discussion_r1746296470)
`OP_RETURN` ?

💬 ajtowns commented on pull request "signet: fixing mining for OP_TRUE challenge":
(https://github.com/bitcoin/bitcoin/pull/29032#discussion_r1746301277)
Better to move `def generate_psbt(..)` down to its code, rather than moving the code up?

💬 ajtowns commented on pull request "signet: fixing mining for OP_TRUE challenge":
(https://github.com/bitcoin/bitcoin/pull/29032#discussion_r1746299406)
nit: I'd probably write this as

```python
if signet_solution is None:
pass # Don't need to add a signet commitment if there's no signet signature needed
else:
...
```

💬 ajtowns commented on pull request "signet: fixing mining for OP_TRUE challenge":
(https://github.com/bitcoin/bitcoin/pull/29032#discussion_r1746305347)
Does it still make sense to grab the `block` from `decode_psbt` here? You already passed it in to `generate_psbt`. Could just be `_, signet_solution = decode_psbt(...)`, or perhaps have separate functions `get_solution_from_psbt()` and `get_block_from_psbt()` ?

💬 ajtowns commented on pull request "signet: fixing mining for OP_TRUE challenge":
(https://github.com/bitcoin/bitcoin/pull/29032#issuecomment-2332893011)
> Using real mining equipment as opposed to a CPU miner is useful, because of the many quirks real devices have (e.g. version bit grinding). Afaik there's no good emulator.

FWIW, I did a patch for bitcoin-util to do version bit grinding: https://github.com/ajtowns/bitcoin/commit/634e72cbfc0aa3f657a35c7b597f688bb2bb29a6 (Note that it lightly depends on the changes in #28802)

💬 gmaxwell commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#issuecomment-2332931281)
Nice benchmarks, any value in running them on the removed optimizations?

> clusterlin: avoid recomputing potential set on every split (optimization)
> clusterlin: avoid jump ahead in unmodified pot sets (optimization)


Any idea why LinearizeSerializedCluster5 is an outlier now? It separates from the rest at "improve heuristic to decide split transaction".

💬 sipa commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1746335009)
Done, pushed new ones, and moved the commit that introduces them forward to the point where they make sense.

💬 sipa commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#issuecomment-2332992189)
@gmaxwell I've just pushed a new set of example benchmark clusters, with the property that they need between 100k and 1M iterations to linearize optimally sometimes and never more than 10M iterations. I'm not well set up right now to benchmark, but at the end of this PR, I see:

| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 13,034,966.68 | 76.72 | 1.2% |
...