💬 Eunovo commented on pull request "descriptors: inference process, do not return unparsable multisig descriptors":
(https://github.com/bitcoin/bitcoin/pull/31404#discussion_r1898893779)
https://github.com/bitcoin/bitcoin/pull/31404/commits/088bbb81e34dfbe56fdeafaba2ddf756017e86e1: `MAX_BARE_MULTISIG_PUBKEYS_NUM` is an unsigned int so you can use `%u` format specifier instead of `%d`
(https://github.com/bitcoin/bitcoin/pull/31404#discussion_r1898893779)
https://github.com/bitcoin/bitcoin/pull/31404/commits/088bbb81e34dfbe56fdeafaba2ddf756017e86e1: `MAX_BARE_MULTISIG_PUBKEYS_NUM` is an unsigned int so you can use `%u` format specifier instead of `%d`
💬 Eunovo commented on pull request "descriptors: inference process, do not return unparsable multisig descriptors":
(https://github.com/bitcoin/bitcoin/pull/31404#discussion_r1898901503)
https://github.com/bitcoin/bitcoin/pull/31404/commits/50ebd73aeeb853328d1f4eadba77f2ca80a27a8c: AFAICT At this point, the `addmultisigaddress` operation is already complete (the scripts have been imported into the wallet and the destination is in the address book), throwing an error here may mislead the user into thinking the operation failed.
If you want the operation to fail if an invalid descriptor is inferred then this code needs to be moved up before the scripts are imported into the wal
...
(https://github.com/bitcoin/bitcoin/pull/31404#discussion_r1898901503)
https://github.com/bitcoin/bitcoin/pull/31404/commits/50ebd73aeeb853328d1f4eadba77f2ca80a27a8c: AFAICT At this point, the `addmultisigaddress` operation is already complete (the scripts have been imported into the wallet and the destination is in the address book), throwing an error here may mislead the user into thinking the operation failed.
If you want the operation to fail if an invalid descriptor is inferred then this code needs to be moved up before the scripts are imported into the wal
...
💬 Eunovo commented on pull request "descriptors: inference process, do not return unparsable multisig descriptors":
(https://github.com/bitcoin/bitcoin/pull/31404#discussion_r1898907274)
https://github.com/bitcoin/bitcoin/pull/31404/commits/50ebd73aeeb853328d1f4eadba77f2ca80a27a8c: The followig callers of ScriptToUniv need to be updated.
```diff
diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
index 4cbe4a6062..5fcb9ff0d2 100644
--- a/src/rpc/blockchain.cpp
+++ b/src/rpc/blockchain.cpp
@@ -1105,7 +1105,7 @@ static RPCHelpMan gettxout()
{RPCResult::Type::STR_AMOUNT, "value", "The transaction value in " + CURRENCY_UNIT},
{RPC
...
(https://github.com/bitcoin/bitcoin/pull/31404#discussion_r1898907274)
https://github.com/bitcoin/bitcoin/pull/31404/commits/50ebd73aeeb853328d1f4eadba77f2ca80a27a8c: The followig callers of ScriptToUniv need to be updated.
```diff
diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
index 4cbe4a6062..5fcb9ff0d2 100644
--- a/src/rpc/blockchain.cpp
+++ b/src/rpc/blockchain.cpp
@@ -1105,7 +1105,7 @@ static RPCHelpMan gettxout()
{RPCResult::Type::STR_AMOUNT, "value", "The transaction value in " + CURRENCY_UNIT},
{RPC
...
📝 Jorah-M opened a pull request: "fix: typos fix in README.md"
(https://github.com/bitcoin/bitcoin/pull/31578)
<!--
*** 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 for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
(https://github.com/bitcoin/bitcoin/pull/31578)
<!--
*** 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 for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
✅ fanquake closed a pull request: "fix: typos fix in README.md"
(https://github.com/bitcoin/bitcoin/pull/31578)
(https://github.com/bitcoin/bitcoin/pull/31578)
💬 furszy commented on pull request "descriptors: inference process, do not return unparsable multisig descriptors":
(https://github.com/bitcoin/bitcoin/pull/31404#discussion_r1898916436)
Sure, good catch. Still, this shouldn't happen because we are creating the destination internally. It is purely a sanity check.
(https://github.com/bitcoin/bitcoin/pull/31404#discussion_r1898916436)
Sure, good catch. Still, this shouldn't happen because we are creating the destination internally. It is purely a sanity check.
💬 furszy commented on pull request "descriptors: inference process, do not return unparsable multisig descriptors":
(https://github.com/bitcoin/bitcoin/pull/31404#discussion_r1898916762)
sure. Applied.
(https://github.com/bitcoin/bitcoin/pull/31404#discussion_r1898916762)
sure. Applied.
💬 furszy commented on pull request "descriptors: inference process, do not return unparsable multisig descriptors":
(https://github.com/bitcoin/bitcoin/pull/31404#discussion_r1898917082)
Sure. Done as suggested. Thanks.
(https://github.com/bitcoin/bitcoin/pull/31404#discussion_r1898917082)
Sure. Done as suggested. Thanks.
👍 TheCharlatan approved a pull request: "validation: Send correct notification during snapshot completion"
(https://github.com/bitcoin/bitcoin/pull/31556#pullrequestreview-2524724192)
lgtm ACK bc43ecaf6dc0830a27296d3a29428814fed07bb1
(https://github.com/bitcoin/bitcoin/pull/31556#pullrequestreview-2524724192)
lgtm ACK bc43ecaf6dc0830a27296d3a29428814fed07bb1
💬 TheCharlatan commented on pull request "validation: Send correct notification during snapshot completion":
(https://github.com/bitcoin/bitcoin/pull/31556#discussion_r1898917833)
Should the docs be updated to mention the added node?
(https://github.com/bitcoin/bitcoin/pull/31556#discussion_r1898917833)
Should the docs be updated to mention the added node?
💬 andrewtoth commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r1898942778)
Adjusted, thanks:
> // It's been a while since we wrote the block index and chain state to disk. Do this frequently, so we don't need to redownload or reindex after a crash.
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r1898942778)
Adjusted, thanks:
> // It's been a while since we wrote the block index and chain state to disk. Do this frequently, so we don't need to redownload or reindex after a crash.
💬 andrewtoth commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#issuecomment-2564363281)
Rebased due to conflicts from #31534.
(https://github.com/bitcoin/bitcoin/pull/30611#issuecomment-2564363281)
Rebased due to conflicts from #31534.
💬 l0rinc commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#issuecomment-2564375695)
ACK b37452ca437856ed5d8effce2cef737d9df479c1
The differences since last ACK:
* the conflicting new logging - which had to be reindented;
* `chain state` and `reindex` mentioned in the comment of `fPeriodicWrite`.
(https://github.com/bitcoin/bitcoin/pull/30611#issuecomment-2564375695)
ACK b37452ca437856ed5d8effce2cef737d9df479c1
The differences since last ACK:
* the conflicting new logging - which had to be reindented;
* `chain state` and `reindex` mentioned in the comment of `fPeriodicWrite`.
👍 tdb3 approved a pull request: "rpc: allow writing UTXO set to a named pipe, introduce dump_to_sqlite.sh script"
(https://github.com/bitcoin/bitcoin/pull/31560#pullrequestreview-2524765817)
ACK 59df8480be77a8e3618c9422536c4f8aed82467e
Also re-ran tests in https://github.com/bitcoin/bitcoin/pull/31560#pullrequestreview-2522042088
(https://github.com/bitcoin/bitcoin/pull/31560#pullrequestreview-2524765817)
ACK 59df8480be77a8e3618c9422536c4f8aed82467e
Also re-ran tests in https://github.com/bitcoin/bitcoin/pull/31560#pullrequestreview-2522042088
👍 tdb3 approved a pull request: "test: descriptor: fix test for `MaxSatisfactionWeight`"
(https://github.com/bitcoin/bitcoin/pull/31570#pullrequestreview-2524768462)
re ACK b29d68f942e333d2cfdd6be8d49fe484e0d15e11
(https://github.com/bitcoin/bitcoin/pull/31570#pullrequestreview-2524768462)
re ACK b29d68f942e333d2cfdd6be8d49fe484e0d15e11
🤔 tdb3 reviewed a pull request: "contrib: add tool to convert compact-serialized UTXO set to SQLite database"
(https://github.com/bitcoin/bitcoin/pull/27432#pullrequestreview-2524872630)
Approach ACK
Providing some manual test results. Planning to review `utxo_to_sqlite.py` in more detail.
Created a utxo dump (for 876819), created the sqlite dump from it, then ran `calc_utxo_hash.go` against it. muhash values matched.
```
$ bitcoin-cli dumptxoutset ~/utxos876819.dat
$ bitcoin-cli gettxoutsetinfo muhash 876819
...
"muhash": "ffb3cbccf78eba08c13f5486b05562f399058023f4f10b593973c448966826f3",
$ contrib/utxo-tools/utxo_to_sqlite.py ~/utxos876819.dat ~/utxos876819.sql
...
(https://github.com/bitcoin/bitcoin/pull/27432#pullrequestreview-2524872630)
Approach ACK
Providing some manual test results. Planning to review `utxo_to_sqlite.py` in more detail.
Created a utxo dump (for 876819), created the sqlite dump from it, then ran `calc_utxo_hash.go` against it. muhash values matched.
```
$ bitcoin-cli dumptxoutset ~/utxos876819.dat
$ bitcoin-cli gettxoutsetinfo muhash 876819
...
"muhash": "ffb3cbccf78eba08c13f5486b05562f399058023f4f10b593973c448966826f3",
$ contrib/utxo-tools/utxo_to_sqlite.py ~/utxos876819.dat ~/utxos876819.sql
...
💬 JeremiahR commented on pull request "p2p: track and use all potential peers for orphan resolution":
(https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1899052947)
typo
```suggestion
// Return even if the peer isn't an orphan resolution candidate. This would be caught by AlreadyHaveTx.
```
(https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1899052947)
typo
```suggestion
// Return even if the peer isn't an orphan resolution candidate. This would be caught by AlreadyHaveTx.
```
💬 Eunovo commented on pull request "descriptors: inference process, do not return unparsable multisig descriptors":
(https://github.com/bitcoin/bitcoin/pull/31404#discussion_r1899085059)
It looks like you have resolved this in https://github.com/bitcoin/bitcoin/pull/31404/commits/e1d6179b61a2d4766c5eb6183a839fcaa6e566fa. Mark this as resolved?
(https://github.com/bitcoin/bitcoin/pull/31404#discussion_r1899085059)
It looks like you have resolved this in https://github.com/bitcoin/bitcoin/pull/31404/commits/e1d6179b61a2d4766c5eb6183a839fcaa6e566fa. Mark this as resolved?
👍 romanz approved a pull request: "contrib: add tool to convert compact-serialized UTXO set to SQLite database"
(https://github.com/bitcoin/bitcoin/pull/27432#pullrequestreview-2524929410)
tACK 4080b66cbe on signet (using [calc_utxo_hash](https://github.com/theStack/utxo_dump_tools/blob/8981aa3e85efac046f0f3b6a1f99d3f4a273cdd1/calc_utxo_hash/calc_utxo_hash.go)):
```
$ bitcoin-cli -signet dumptxoutset signet.utxo latest
{
"coins_written": 5898081,
"base_hash": "0000007e74079165369e130c7df56bcfa8100f64049d82d69b8fdce6fe95644f",
"base_height": 228463,
"path": "/home/user/.bitcoin/signet/signet.utxo",
"txoutset_hash": "3ce62cf97d0acbb35e3e8d822760df08b8eb7bcc98886c8b
...
(https://github.com/bitcoin/bitcoin/pull/27432#pullrequestreview-2524929410)
tACK 4080b66cbe on signet (using [calc_utxo_hash](https://github.com/theStack/utxo_dump_tools/blob/8981aa3e85efac046f0f3b6a1f99d3f4a273cdd1/calc_utxo_hash/calc_utxo_hash.go)):
```
$ bitcoin-cli -signet dumptxoutset signet.utxo latest
{
"coins_written": 5898081,
"base_hash": "0000007e74079165369e130c7df56bcfa8100f64049d82d69b8fdce6fe95644f",
"base_height": 228463,
"path": "/home/user/.bitcoin/signet/signet.utxo",
"txoutset_hash": "3ce62cf97d0acbb35e3e8d822760df08b8eb7bcc98886c8b
...
💬 jafpri1967 commented on pull request "contrib: add tool to convert compact-serialized UTXO set to SQLite database":
(https://github.com/bitcoin/bitcoin/pull/27432#issuecomment-2564733016)
>
(https://github.com/bitcoin/bitcoin/pull/27432#issuecomment-2564733016)
>