💬 fanquake commented on pull request "[27.x] Even more backports":
(https://github.com/bitcoin/bitcoin/pull/30558#discussion_r1713746902)
Added.
(https://github.com/bitcoin/bitcoin/pull/30558#discussion_r1713746902)
Added.
👍 hodlinator approved a pull request: "contrib: support reading XORed blocks in linearize-data.py script"
(https://github.com/bitcoin/bitcoin/pull/30607#pullrequestreview-2232995233)
ACK 77ff0ec1f185b818b30877de2bedc1750319e6c4
Good to see follow-up work for #28052 being carried out.
1cbc5ae87a8efe44226080ceaaaf10c5575d629d explains purpose of linearize-scripts (as it was not immediately apparent to me). Could be cherry-picked into this PR, or left for a further follow-up.
1. `bitcoind -regtest &`
2. `bitcoin-cli -regtest getblockcount` returns height=120.
3. `contrib/linearize/linearize-hashes.py contrib/linearize/example-linearize.cfg > hashlist.txt` (with modif
...
(https://github.com/bitcoin/bitcoin/pull/30607#pullrequestreview-2232995233)
ACK 77ff0ec1f185b818b30877de2bedc1750319e6c4
Good to see follow-up work for #28052 being carried out.
1cbc5ae87a8efe44226080ceaaaf10c5575d629d explains purpose of linearize-scripts (as it was not immediately apparent to me). Could be cherry-picked into this PR, or left for a further follow-up.
1. `bitcoind -regtest &`
2. `bitcoin-cli -regtest getblockcount` returns height=120.
3. `contrib/linearize/linearize-hashes.py contrib/linearize/example-linearize.cfg > hashlist.txt` (with modif
...
💬 fjahr commented on pull request "test: assumeutxo: check that UTXO-querying RPCs operate on snapshot chainstate":
(https://github.com/bitcoin/bitcoin/pull/30636#issuecomment-2283996399)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/30636#issuecomment-2283996399)
Concept ACK
💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1713786718)
Also https://github.com/hebasto/bitcoin/pull/324.
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1713786718)
Also https://github.com/hebasto/bitcoin/pull/324.
💬 hebasto commented on pull request "contrib: fix check-deps.sh to check for weak symbols":
(https://github.com/bitcoin/bitcoin/pull/30415#issuecomment-2284008706)
cc @maflcko @fanquake
(https://github.com/bitcoin/bitcoin/pull/30415#issuecomment-2284008706)
cc @maflcko @fanquake
💬 Sjors commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange and drop CRPCSignals":
(https://github.com/bitcoin/bitcoin/pull/30409#issuecomment-2284019673)
I simplified the `waitforblock` and `waitfornewblock` tests.
They're not very thorough at the moment, because `reconsiderblock` is either atomic or too fast. A better way to test the asynchronous behaviour would be to spin up a second node, and feed it blocks through `add_p2p_connection`.
(https://github.com/bitcoin/bitcoin/pull/30409#issuecomment-2284019673)
I simplified the `waitforblock` and `waitfornewblock` tests.
They're not very thorough at the moment, because `reconsiderblock` is either atomic or too fast. A better way to test the asynchronous behaviour would be to spin up a second node, and feed it blocks through `add_p2p_connection`.
💬 furszy commented on issue "An "output descriptor" should not have many different checksums":
(https://github.com/bitcoin/bitcoin/issues/30632#issuecomment-2284066800)
The goal of the checksum is to protect the human-readable string from data transmission errors, not to establish a global output identifier. You can specify the same output script in different ways, and this doesn't mean they will share the same checksum. Look:
1. Your descriptor `"wpkh([b8688df1]xprv9s21ZrQH143K2x4gnzRB1eZDq92Uuvy9CXbvgQGdvykXZ9mkkot6LBjzDpgaAfvzkuxJe9JKJXQ38VoPutxvACA5MsyoBs5UyQ4HZKGshGs/84'/0'/0'/0/0)#mpfvuvu6"`
2. Descriptor without origin `"wpkh(xprv9s21ZrQH143K2x4gnzRB
...
(https://github.com/bitcoin/bitcoin/issues/30632#issuecomment-2284066800)
The goal of the checksum is to protect the human-readable string from data transmission errors, not to establish a global output identifier. You can specify the same output script in different ways, and this doesn't mean they will share the same checksum. Look:
1. Your descriptor `"wpkh([b8688df1]xprv9s21ZrQH143K2x4gnzRB1eZDq92Uuvy9CXbvgQGdvykXZ9mkkot6LBjzDpgaAfvzkuxJe9JKJXQ38VoPutxvACA5MsyoBs5UyQ4HZKGshGs/84'/0'/0'/0/0)#mpfvuvu6"`
2. Descriptor without origin `"wpkh(xprv9s21ZrQH143K2x4gnzRB
...
💬 rkrux commented on pull request "BlockAssembler: return selected packages vsize and feerate":
(https://github.com/bitcoin/bitcoin/pull/30391#discussion_r1713862201)
Thank you for accepting the suggestion, the diff now has become smaller and to the point.
(https://github.com/bitcoin/bitcoin/pull/30391#discussion_r1713862201)
Thank you for accepting the suggestion, the diff now has become smaller and to the point.
👍 rkrux approved a pull request: "BlockAssembler: return selected packages vsize and feerate"
(https://github.com/bitcoin/bitcoin/pull/30391#pullrequestreview-2233172836)
reACK [8a830f1](https://github.com/bitcoin/bitcoin/commit/8a830f15cf4923f57abf0bdf30b98b04b47c6e7b)
(https://github.com/bitcoin/bitcoin/pull/30391#pullrequestreview-2233172836)
reACK [8a830f1](https://github.com/bitcoin/bitcoin/commit/8a830f15cf4923f57abf0bdf30b98b04b47c6e7b)
🤔 glozow reviewed a pull request: "wallet: Fix listwalletdir listing of migrated default wallets and generated backup files"
(https://github.com/bitcoin/bitcoin/pull/30265#pullrequestreview-2233246739)
ACK 6b2dcba07670f04f32c0dc3a2c86fd805c85f12d
Tested by creating legacy default and non-default wallets, then migrating, then using `listwalletdir`. I can see the default wallet is listed as a .bak on master, then disappears on 3ddbdd1815c676a88345b3b0e55a551d2a569e28, then is listed with its name on 6b2dcba07670f04f32c0dc3a2c86fd805c85f12d.
(https://github.com/bitcoin/bitcoin/pull/30265#pullrequestreview-2233246739)
ACK 6b2dcba07670f04f32c0dc3a2c86fd805c85f12d
Tested by creating legacy default and non-default wallets, then migrating, then using `listwalletdir`. I can see the default wallet is listed as a .bak on master, then disappears on 3ddbdd1815c676a88345b3b0e55a551d2a569e28, then is listed with its name on 6b2dcba07670f04f32c0dc3a2c86fd805c85f12d.
💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2284184677)
@fanquake [wrote](https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2277886777):
> I think we can improve the output when `-DWITH_CCACHE=OFF` is used. Depending on the system, that output might be:
>
> ```shell
> cmake -B build -DWITH_CCACHE=OFF
> < snip >
> Use ccache for compiling .............. ccache masquerades as the compiler
> ```
>
> We should probably at least indicate that the option was respected by the build-system.
The issue, along with another bug I've notice
...
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2284184677)
@fanquake [wrote](https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2277886777):
> I think we can improve the output when `-DWITH_CCACHE=OFF` is used. Depending on the system, that output might be:
>
> ```shell
> cmake -B build -DWITH_CCACHE=OFF
> < snip >
> Use ccache for compiling .............. ccache masquerades as the compiler
> ```
>
> We should probably at least indicate that the option was respected by the build-system.
The issue, along with another bug I've notice
...
🤔 fjahr reviewed a pull request: "test: assumeutxo: check that UTXO-querying RPCs operate on snapshot chainstate"
(https://github.com/bitcoin/bitcoin/pull/30636#pullrequestreview-2233277476)
Code review ACK 2e9072c137e81c75c58d0c0788295c10fdafdc9b
This is a good regression test to have. I left a bunch of minor nits but this is fine to merge as-is so feel free to leave them unless you have to re-touch.
(https://github.com/bitcoin/bitcoin/pull/30636#pullrequestreview-2233277476)
Code review ACK 2e9072c137e81c75c58d0c0788295c10fdafdc9b
This is a good regression test to have. I left a bunch of minor nits but this is fine to merge as-is so feel free to leave them unless you have to re-touch.
💬 fjahr commented on pull request "test: assumeutxo: check that UTXO-querying RPCs operate on snapshot chainstate":
(https://github.com/bitcoin/bitcoin/pull/30636#discussion_r1713929119)
nit
```suggestion
assert_equal(scan_result['bestblock'], snapshot_hash)
```
(https://github.com/bitcoin/bitcoin/pull/30636#discussion_r1713929119)
nit
```suggestion
assert_equal(scan_result['bestblock'], snapshot_hash)
```
💬 fjahr commented on pull request "test: assumeutxo: check that UTXO-querying RPCs operate on snapshot chainstate":
(https://github.com/bitcoin/bitcoin/pull/30636#discussion_r1713932403)
nit: Would be a bit more readable IMO if you would break it into two lines
(https://github.com/bitcoin/bitcoin/pull/30636#discussion_r1713932403)
nit: Would be a bit more readable IMO if you would break it into two lines
💬 fjahr commented on pull request "test: assumeutxo: check that UTXO-querying RPCs operate on snapshot chainstate":
(https://github.com/bitcoin/bitcoin/pull/30636#discussion_r1713927674)
nit: could have moved this up and used it in the `gettxoutsetinfo` as well
(https://github.com/bitcoin/bitcoin/pull/30636#discussion_r1713927674)
nit: could have moved this up and used it in the `gettxoutsetinfo` as well
💬 fjahr commented on pull request "test: assumeutxo: check that UTXO-querying RPCs operate on snapshot chainstate":
(https://github.com/bitcoin/bitcoin/pull/30636#discussion_r1713931438)
On second thought: maybe just pull all three of these loaded keys out in the beginning since you use each of them at least twice and they are used again further below... or you could deduplicate the `assert_equal` triplet but it's not a big deal either way...
(https://github.com/bitcoin/bitcoin/pull/30636#discussion_r1713931438)
On second thought: maybe just pull all three of these loaded keys out in the beginning since you use each of them at least twice and they are used again further below... or you could deduplicate the `assert_equal` triplet but it's not a big deal either way...
🚀 glozow merged a pull request: "wallet: Fix listwalletdir listing of migrated default wallets and generated backup files"
(https://github.com/bitcoin/bitcoin/pull/30265)
(https://github.com/bitcoin/bitcoin/pull/30265)
💬 mzumsande commented on pull request "addrman: change internal id counting to int64_t":
(https://github.com/bitcoin/bitcoin/pull/30568#issuecomment-2284290341)
> Concept ACK. if you retouch, there's also:
Good catch, fixed!
(https://github.com/bitcoin/bitcoin/pull/30568#issuecomment-2284290341)
> Concept ACK. if you retouch, there's also:
Good catch, fixed!
🤔 furszy reviewed a pull request: "Migrate legacy wallets that are not loaded"
(https://github.com/bitcoin-core/gui/pull/824#pullrequestreview-2231855105)
Few comments. Will continue reviewing.
(https://github.com/bitcoin-core/gui/pull/824#pullrequestreview-2231855105)
Few comments. Will continue reviewing.
💬 furszy commented on pull request "Migrate legacy wallets that are not loaded":
(https://github.com/bitcoin-core/gui/pull/824#discussion_r1713021114)
I don't think showing the already migrated wallets here adds any value. The user cannot do anything with them and they do not present any extra information. I would skip them:
```diff
diff --git a/src/qt/bitcoingui.cpp b/src/qt/bitcoingui.cpp
--- a/src/qt/bitcoingui.cpp
+++ b/src/qt/bitcoingui.cpp
@@ -460,6 +460,10 @@
m_migrate_wallet_menu->clear();
for (const auto& [wallet_name, info] : m_wallet_controller->listWalletDir()) {
const auto& [loade
...
(https://github.com/bitcoin-core/gui/pull/824#discussion_r1713021114)
I don't think showing the already migrated wallets here adds any value. The user cannot do anything with them and they do not present any extra information. I would skip them:
```diff
diff --git a/src/qt/bitcoingui.cpp b/src/qt/bitcoingui.cpp
--- a/src/qt/bitcoingui.cpp
+++ b/src/qt/bitcoingui.cpp
@@ -460,6 +460,10 @@
m_migrate_wallet_menu->clear();
for (const auto& [wallet_name, info] : m_wallet_controller->listWalletDir()) {
const auto& [loade
...