💬 ismaelsadeeq commented on pull request "Wallet, Bugfix: Lock Wallet Context mutex Before Adding/Removing Settings":
(https://github.com/bitcoin/bitcoin/pull/30697#discussion_r1727772841)
This is fixed.
(https://github.com/bitcoin/bitcoin/pull/30697#discussion_r1727772841)
This is fixed.
💬 TheCharlatan commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange and drop CRPCSignals":
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1727777786)
Couple of months ago I was thinking how to get rid of `g_best_block` and its friends entirely in the context of the kernel library. We already have two interfaces for notifying if something changed during validation, having a third one, that is also globally mutable and potentially dangerous if multiple chainstate managers are used is not ideal. I did not make progress then, but looking at it again now, I think this might be the opportunity to get rid of it.
How about duplicating the best blo
...
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1727777786)
Couple of months ago I was thinking how to get rid of `g_best_block` and its friends entirely in the context of the kernel library. We already have two interfaces for notifying if something changed during validation, having a third one, that is also globally mutable and potentially dangerous if multiple chainstate managers are used is not ideal. I did not make progress then, but looking at it again now, I think this might be the opportunity to get rid of it.
How about duplicating the best blo
...
💬 l0rinc commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1727782615)
```suggestion
* use `std::byte` instead of `unsigned char` and `uint8_t`.
```
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1727782615)
```suggestion
* use `std::byte` instead of `unsigned char` and `uint8_t`.
```
💬 cryptoquick commented on issue "Fatal LevelDB error: Corruption: block checksum mismatch on Linux ext4 SATA SSDs":
(https://github.com/bitcoin/bitcoin/issues/30692#issuecomment-2305593889)
I ran into the error earlier on in the IBD this time. It seems like there's no rhyme or reason as to why this is occurring.
```
2024-08-22T12:58:13Z UpdateTip: new best=0000000000000000011a06e4b1a3c497d247f785a4899e44ae800a9236438e16 height=424472 version=0x20000000 log2_work=85.108754 tx=148148770 date='2016-08-10T00:15:52Z' progress=0.137300 cache=124.8MiB(1146194txo)
2024-08-22T12:58:13Z UpdateTip: new best=0000000000000000003a58cdf5401248a1330480a1c9b99440a5f974fb61ce17 height=424473 ve
...
(https://github.com/bitcoin/bitcoin/issues/30692#issuecomment-2305593889)
I ran into the error earlier on in the IBD this time. It seems like there's no rhyme or reason as to why this is occurring.
```
2024-08-22T12:58:13Z UpdateTip: new best=0000000000000000011a06e4b1a3c497d247f785a4899e44ae800a9236438e16 height=424472 version=0x20000000 log2_work=85.108754 tx=148148770 date='2016-08-10T00:15:52Z' progress=0.137300 cache=124.8MiB(1146194txo)
2024-08-22T12:58:13Z UpdateTip: new best=0000000000000000003a58cdf5401248a1330480a1c9b99440a5f974fb61ce17 height=424473 ve
...
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1727788962)
```diff
--- before 2024-08-22 22:41:41.949431649 +0200
+++ after 2024-08-22 22:41:56.957571134 +0200
@@ -1,7 +1,7 @@
-/**
+/**
* ""_hex is a compile-time user-defined literal returning a
- * `std::array<std::byte>`, equivalent to `ParseHex`. Variants include:
- *
+ * `std::array<std::byte>`, equivalent to ParseHex(). Variants provided:
+ *
* - ""_hex_v: Returns `std::vector<std::byte>`, useful for heap allocation or
* variable-length serialization.
*
@@ -21,5 +21,8 @@
...
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1727788962)
```diff
--- before 2024-08-22 22:41:41.949431649 +0200
+++ after 2024-08-22 22:41:56.957571134 +0200
@@ -1,7 +1,7 @@
-/**
+/**
* ""_hex is a compile-time user-defined literal returning a
- * `std::array<std::byte>`, equivalent to `ParseHex`. Variants include:
- *
+ * `std::array<std::byte>`, equivalent to ParseHex(). Variants provided:
+ *
* - ""_hex_v: Returns `std::vector<std::byte>`, useful for heap allocation or
* variable-length serialization.
*
@@ -21,5 +21,8 @@
...
📝 l0rinc opened a pull request: "CI: fix 3 simple codespell warnings"
(https://github.com/bitcoin/bitcoin/pull/30700)
Seen while investigating https://github.com/bitcoin/bitcoin/pull/30377/checks?check_run_id=29134897722:
* src/consensus/params.h:112: Enfore ==> Enforce
* src/test/fuzz/utxo_snapshot.cpp:75: Re-use ==> Reuse
* src/test/util/cluster_linearize.h:158: encounted ==> encountered, encounter
(https://github.com/bitcoin/bitcoin/pull/30700)
Seen while investigating https://github.com/bitcoin/bitcoin/pull/30377/checks?check_run_id=29134897722:
* src/consensus/params.h:112: Enfore ==> Enforce
* src/test/fuzz/utxo_snapshot.cpp:75: Re-use ==> Reuse
* src/test/util/cluster_linearize.h:158: encounted ==> encountered, encounter
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1727790062)
Just spotted the whitespace at end of line.. fixing. :)
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1727790062)
Just spotted the whitespace at end of line.. fixing. :)
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1727794829)
("ParseHex()" becomes a link in Doxygen).
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1727794829)
("ParseHex()" becomes a link in Doxygen).
💬 l0rinc commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2305609525)
ACK df92661444f46790b12d5061344d72106ef820d9
Doc updates
(https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2305609525)
ACK df92661444f46790b12d5061344d72106ef820d9
Doc updates
💬 theStack commented on pull request "test: XORed blocks test follow up":
(https://github.com/bitcoin/bitcoin/pull/30669#discussion_r1727850357)
Not sure if there are guidelines for this (and if yes, how strict we are), but I would tend to avoid using the "property" decorator for non-trivial methods that access state outside of the class (in this case, by interacting with the file system). At least I wouldn't expect as user that accessing a class field ever involves any I/O. I think dropping the property and keeping the name as-is "read_xor_key" might be just fine?
(https://github.com/bitcoin/bitcoin/pull/30669#discussion_r1727850357)
Not sure if there are guidelines for this (and if yes, how strict we are), but I would tend to avoid using the "property" decorator for non-trivial methods that access state outside of the class (in this case, by interacting with the file system). At least I wouldn't expect as user that accessing a class field ever involves any I/O. I think dropping the property and keeping the name as-is "read_xor_key" might be just fine?
💬 theStack commented on pull request "test: XORed blocks test follow up":
(https://github.com/bitcoin/bitcoin/pull/30669#discussion_r1727856174)
nit: could put this already into the commit that introduces the blocks_key_path property to TestNode.
(https://github.com/bitcoin/bitcoin/pull/30669#discussion_r1727856174)
nit: could put this already into the commit that introduces the blocks_key_path property to TestNode.
🤔 furszy reviewed a pull request: "init: fix init fatal error on invalid negated option value"
(https://github.com/bitcoin/bitcoin/pull/30684#pullrequestreview-2255865283)
Hmm yeah ok, nice catch. I missed that `getSettingsList` return a list of json that can be checked against the string type.
One drawback of your suggestion is that we currently fail (crash) when the invalid value is provided and with your suggestion, we would ignore it. So, by following-up this fix with new args restrictions (which I think we should do) we would be changing behavior twice. Unless we don't backport the crash fix and merge both PRs during the same release window or.. we fail with
...
(https://github.com/bitcoin/bitcoin/pull/30684#pullrequestreview-2255865283)
Hmm yeah ok, nice catch. I missed that `getSettingsList` return a list of json that can be checked against the string type.
One drawback of your suggestion is that we currently fail (crash) when the invalid value is provided and with your suggestion, we would ignore it. So, by following-up this fix with new args restrictions (which I think we should do) we would be changing behavior twice. Unless we don't backport the crash fix and merge both PRs during the same release window or.. we fail with
...
📝 martinsaposnic opened a pull request: "Use MiniWallet in functional test rpc_signrawtransactionwithkey."
(https://github.com/bitcoin/bitcoin/pull/30701)
In response to issue https://github.com/bitcoin/bitcoin/issues/30600, optimizations have been implemented to enhance test efficiency and readability:
1. Simplified `send_to_address` Method:
The `send_to_address` method has been refactored to utilize MiniWallet. This change significantly reduces unnecessary complexity and improves performance. By abstracting away the low-level transaction details of funding transaction creation, the test code becomes more concise and easier to understand.
...
(https://github.com/bitcoin/bitcoin/pull/30701)
In response to issue https://github.com/bitcoin/bitcoin/issues/30600, optimizations have been implemented to enhance test efficiency and readability:
1. Simplified `send_to_address` Method:
The `send_to_address` method has been refactored to utilize MiniWallet. This change significantly reduces unnecessary complexity and improves performance. By abstracting away the low-level transaction details of funding transaction creation, the test code becomes more concise and easier to understand.
...
💬 martinsaposnic commented on issue "use MiniWallet in functional test `rpc_signrawtransactionwithkey.py` for funding txs":
(https://github.com/bitcoin/bitcoin/issues/30600#issuecomment-2305817413)
new possible solution for this issue: https://github.com/bitcoin/bitcoin/pull/30701 🙂
(https://github.com/bitcoin/bitcoin/issues/30600#issuecomment-2305817413)
new possible solution for this issue: https://github.com/bitcoin/bitcoin/pull/30701 🙂
💬 furszy commented on pull request "init: fix init fatal error on invalid negated option value":
(https://github.com/bitcoin/bitcoin/pull/30684#issuecomment-2305829414)
Found #16545 at the `ArgsManager::Flags` enum when started implementing the same idea. Cool stuff. Happy to move there on this release cycle. Will report a general error at the two `if (!wallet.isStr())` lines to fix the crash and keep the init failure here.
(https://github.com/bitcoin/bitcoin/pull/30684#issuecomment-2305829414)
Found #16545 at the `ArgsManager::Flags` enum when started implementing the same idea. Cool stuff. Happy to move there on this release cycle. Will report a general error at the two `if (!wallet.isStr())` lines to fix the crash and keep the init failure here.
💬 ryanofsky commented on pull request "init: fix init fatal error on invalid negated option value":
(https://github.com/bitcoin/bitcoin/pull/30684#issuecomment-2305863821)
I'm not sure I understand the drawback of the suggested fix in https://github.com/bitcoin/bitcoin/pull/30684#issuecomment-2299869398. You are concerned that a writing `-nowallet=0` or `-nowallet=not_a_bool` would be ignored instead of treated as an error, and then potentially be treated as an error again at some future date?
This doesn't seem like big problem to me because `-nosetting=0` `-nosetting=not_a_bool` is already allowed most other places and it is just treated `-setting=1`. So it se
...
(https://github.com/bitcoin/bitcoin/pull/30684#issuecomment-2305863821)
I'm not sure I understand the drawback of the suggested fix in https://github.com/bitcoin/bitcoin/pull/30684#issuecomment-2299869398. You are concerned that a writing `-nowallet=0` or `-nowallet=not_a_bool` would be ignored instead of treated as an error, and then potentially be treated as an error again at some future date?
This doesn't seem like big problem to me because `-nosetting=0` `-nosetting=not_a_bool` is already allowed most other places and it is just treated `-setting=1`. So it se
...
💬 furszy commented on pull request "init: fix init fatal error on invalid negated option value":
(https://github.com/bitcoin/bitcoin/pull/30684#issuecomment-2305881082)
> I'm not sure I understand the drawback of the suggested fix in [#30684 (comment)](https://github.com/bitcoin/bitcoin/pull/30684#issuecomment-2299869398). You are concerned that a writing `-nowallet=0` or `-nowallet=not_a_bool` would be ignored instead of treated as an error, and then potentially be treated as an error again at some future date?
Yeah, I'm just trying to avoid users placing something like this in the config file, forgetting about it, and then coming back in the future (after
...
(https://github.com/bitcoin/bitcoin/pull/30684#issuecomment-2305881082)
> I'm not sure I understand the drawback of the suggested fix in [#30684 (comment)](https://github.com/bitcoin/bitcoin/pull/30684#issuecomment-2299869398). You are concerned that a writing `-nowallet=0` or `-nowallet=not_a_bool` would be ignored instead of treated as an error, and then potentially be treated as an error again at some future date?
Yeah, I'm just trying to avoid users placing something like this in the config file, forgetting about it, and then coming back in the future (after
...
🤔 theStack reviewed a pull request: "Use MiniWallet in functional test rpc_signrawtransactionwithkey."
(https://github.com/bitcoin/bitcoin/pull/30701#pullrequestreview-2255954132)
Thanks for picking this up!
A few more suggestions to reduce the code even further:
* the initialization of previously needed instance members (`self.blk_idx`, `self.block_hash`) can now be removed
* instead of starting with a clean chain (see `self.setup_clean_chain`), you could use the pre-generated test chain to avoid the need of generating blocks; MiniWallet will pick the relevant UTXOs up automically at instantiation. IIUC it shouldn't be necessary to mine any blocks for this tests
*
...
(https://github.com/bitcoin/bitcoin/pull/30701#pullrequestreview-2255954132)
Thanks for picking this up!
A few more suggestions to reduce the code even further:
* the initialization of previously needed instance members (`self.blk_idx`, `self.block_hash`) can now be removed
* instead of starting with a clean chain (see `self.setup_clean_chain`), you could use the pre-generated test chain to avoid the need of generating blocks; MiniWallet will pick the relevant UTXOs up automically at instantiation. IIUC it shouldn't be necessary to mine any blocks for this tests
*
...
💬 kevkevinpal commented on pull request "CI: fix 3 simple codespell warnings":
(https://github.com/bitcoin/bitcoin/pull/30700#issuecomment-2305907411)
Can you update the title and commit to start with `doc:` instead of `CI:`?
since this is just a doc change
(https://github.com/bitcoin/bitcoin/pull/30700#issuecomment-2305907411)
Can you update the title and commit to start with `doc:` instead of `CI:`?
since this is just a doc change
💬 kevkevinpal commented on pull request "test: Improve clarity of subsidy limit test":
(https://github.com/bitcoin/bitcoin/pull/30699#issuecomment-2305923063)
I don't understand why we need to refactor the whole test, I think additional asserts I'm also neutral on
I think adding asserts without refactoring the test would be better
(https://github.com/bitcoin/bitcoin/pull/30699#issuecomment-2305923063)
I don't understand why we need to refactor the whole test, I think additional asserts I'm also neutral on
I think adding asserts without refactoring the test would be better