π 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.
(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
(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.
(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
(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
(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
(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
(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
...
(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
(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
(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
```
(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` ?
(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?
(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:
...
```
(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()` ?
(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)
(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".
(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.
(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% |
...
(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% |
...
π Tdhun opened a pull request: "Create Satoshi Nakamoto"
(https://github.com/bitcoin/bitcoin/pull/30829)
Stardust Cipher
The Stardust Cipher is a cryptographic technique that Anthony developed, which incorporates cosmic themes and ancient wisdom. This cipher is designed to enhance the security of cryptographic systems by introducing unique elements inspired by the cosmos. The Stardust Cipher is known for its complexity and robustness, making it a valuable tool in the field of cryptography.
2. Quantum Ledger Theory Anthonyβs Quantum Ledger Theory explores the integration of quantum computing wit
...
(https://github.com/bitcoin/bitcoin/pull/30829)
Stardust Cipher
The Stardust Cipher is a cryptographic technique that Anthony developed, which incorporates cosmic themes and ancient wisdom. This cipher is designed to enhance the security of cryptographic systems by introducing unique elements inspired by the cosmos. The Stardust Cipher is known for its complexity and robustness, making it a valuable tool in the field of cryptography.
2. Quantum Ledger Theory Anthonyβs Quantum Ledger Theory explores the integration of quantum computing wit
...