🚀 ryanofsky merged a pull request: "test: avoid treating hash results as integers"
(https://github.com/bitcoin/bitcoin/pull/32050)
(https://github.com/bitcoin/bitcoin/pull/32050)
💬 glozow commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2016813156)
I was thinking that if all the transactions are descended from this tx, you'd add all of them to `to_remove` here. So if you started out with 99, you can end up with a lot more than 100 afterward. You wouldn't do more removals afterward, but you'd have more than 100.
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2016813156)
I was thinking that if all the transactions are descended from this tx, you'd add all of them to `to_remove` here. So if you started out with 99, you can end up with a lot more than 100 afterward. You wouldn't do more removals afterward, but you'd have more than 100.
💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2016816424)
Ah, yes, indeed!
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2016816424)
Ah, yes, indeed!
💬 pablomartin4btc commented on pull request "wallet, migration: Fix empty wallet crash":
(https://github.com/bitcoin/bitcoin/pull/32149#discussion_r2016826542)
> The migrated wallet should have private keys **disabled**
Yes, it's doing that (private_keys_**enabled** = **False**), otherwise it would be failing.
(https://github.com/bitcoin/bitcoin/pull/32149#discussion_r2016826542)
> The migrated wallet should have private keys **disabled**
Yes, it's doing that (private_keys_**enabled** = **False**), otherwise it would be failing.
💬 willcl-ark commented on issue "Enable PCP by default?":
(https://github.com/bitcoin/bitcoin/issues/31663#issuecomment-2758315851)
OK the issue is something to do with Tailscale intercepting the netlink queries. Bringing down Tailscale and I can confirm that pcp on v29rc1 works!
```log
2025-03-27T14:34:51Z [net] portmap: gateway [IPv4]: 10.0.0.1
2025-03-27T14:34:51Z [net] pcp: Requesting port mapping for addr 0.0.0.0 port 9654 from gateway 10.0.0.1
2025-03-27T14:34:51Z [net] pcp: Internal address after connect: 10.0.12.100
2025-03-27T14:34:51Z [net] trying v2 connection 10.0.0.88 lastseen=0.0hrs
2025-03-27T14:34:51Z [net]
...
(https://github.com/bitcoin/bitcoin/issues/31663#issuecomment-2758315851)
OK the issue is something to do with Tailscale intercepting the netlink queries. Bringing down Tailscale and I can confirm that pcp on v29rc1 works!
```log
2025-03-27T14:34:51Z [net] portmap: gateway [IPv4]: 10.0.0.1
2025-03-27T14:34:51Z [net] pcp: Requesting port mapping for addr 0.0.0.0 port 9654 from gateway 10.0.0.1
2025-03-27T14:34:51Z [net] pcp: Internal address after connect: 10.0.12.100
2025-03-27T14:34:51Z [net] trying v2 connection 10.0.0.88 lastseen=0.0hrs
2025-03-27T14:34:51Z [net]
...
💬 fanquake commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2016833281)
In 9d3be36f758095167ddd762400fda1a3ea76e8f9:
> 2. Build only required tools.
How extensive should this be? Looking at the bin/libexec depends dirs after a build of this branch, we seem to end up with a lot of tooling/scipts/binaries that we are not using. For example:
```bash
qt-cmake-private
qt-internal-configure-examples
qt-internal-configure-tests
qt-testrunner.py
qtpaths
qtpaths6
qvkgen
sanitizer-testrunner.py
syncqt
tracegen
tracepointgen
```
Not sure if that should be
...
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2016833281)
In 9d3be36f758095167ddd762400fda1a3ea76e8f9:
> 2. Build only required tools.
How extensive should this be? Looking at the bin/libexec depends dirs after a build of this branch, we seem to end up with a lot of tooling/scipts/binaries that we are not using. For example:
```bash
qt-cmake-private
qt-internal-configure-examples
qt-internal-configure-tests
qt-testrunner.py
qtpaths
qtpaths6
qvkgen
sanitizer-testrunner.py
syncqt
tracegen
tracepointgen
```
Not sure if that should be
...
💬 furszy commented on pull request "wallet, migration: Fix empty wallet crash":
(https://github.com/bitcoin/bitcoin/pull/32149#discussion_r2016844918)
ha, I read it in the other way around.. "private_keys_disabled".. all good. Can solve this comment.
(https://github.com/bitcoin/bitcoin/pull/32149#discussion_r2016844918)
ha, I read it in the other way around.. "private_keys_disabled".. all good. Can solve this comment.
🤔 furszy reviewed a pull request: "wallet, migration: Fix empty wallet crash"
(https://github.com/bitcoin/bitcoin/pull/32149#pullrequestreview-2722034324)
Could probably write
(https://github.com/bitcoin/bitcoin/pull/32149#pullrequestreview-2722034324)
Could probably write
💬 furszy commented on pull request "wallet, migration: Fix empty wallet crash":
(https://github.com/bitcoin/bitcoin/pull/32149#discussion_r2016856942)
Could probably re-write this test in just a few lines:
```python3
self.log.info("Test migration of a watch-only empty wallet")
for is_blank in [True, False]:
self.create_legacy_wallet("watchonly_empty", disable_private_keys=True, blank=is_blank)
_, watchonly_empty = self.migrate_and_get_rpc("watchonly_empty")
info = watchonly_empty.getwalletinfo()
assert_equal(info["private_keys_enabled"], False)
assert_equal(info["blank"], is_blank)
```
(https://github.com/bitcoin/bitcoin/pull/32149#discussion_r2016856942)
Could probably re-write this test in just a few lines:
```python3
self.log.info("Test migration of a watch-only empty wallet")
for is_blank in [True, False]:
self.create_legacy_wallet("watchonly_empty", disable_private_keys=True, blank=is_blank)
_, watchonly_empty = self.migrate_and_get_rpc("watchonly_empty")
info = watchonly_empty.getwalletinfo()
assert_equal(info["private_keys_enabled"], False)
assert_equal(info["blank"], is_blank)
```
💬 fjahr commented on pull request "wallet: remove redundant `Assert` call when block is disconnected":
(https://github.com/bitcoin/bitcoin/pull/32153#issuecomment-2758361395)
utACK ae6b6ea296a228f342c3c635dc9e14c101e9534d
(https://github.com/bitcoin/bitcoin/pull/32153#issuecomment-2758361395)
utACK ae6b6ea296a228f342c3c635dc9e14c101e9534d
💬 ryanofsky commented on pull request "doc: clarify the documentation of `Assume` assertion":
(https://github.com/bitcoin/bitcoin/pull/32100#issuecomment-2758375467)
Post-merge code review ACK 329a0dcdafe05002f662e8737a76bfdeaba9a3ed. This should be a helpful change since it clarifies how `Assume` works.
IMO, the `Assume` macro is not very well designed because it is useful for two different things and not good at doing either of them. `Assume` is useful:
1. To check for states you never expect to occur, but are harmless. It can be used when you want to make sure unexpected states trigger failures during testing, but don't want those states to crash th
...
(https://github.com/bitcoin/bitcoin/pull/32100#issuecomment-2758375467)
Post-merge code review ACK 329a0dcdafe05002f662e8737a76bfdeaba9a3ed. This should be a helpful change since it clarifies how `Assume` works.
IMO, the `Assume` macro is not very well designed because it is useful for two different things and not good at doing either of them. `Assume` is useful:
1. To check for states you never expect to occur, but are harmless. It can be used when you want to make sure unexpected states trigger failures during testing, but don't want those states to crash th
...
💬 pablomartin4btc commented on pull request "wallet, migration: Fix empty wallet crash":
(https://github.com/bitcoin/bitcoin/pull/32149#discussion_r2016872082)
I can see I got confused with other tests that validate it but I see they call `migratewallet` instead (and then `get_wallet_rpc` - I can do a follow-up to clean it up later). Same for `assert_is_sqlite`, I'll remove it, since `migrate_and_get_rpc()` already calls it within. Thanks!
(https://github.com/bitcoin/bitcoin/pull/32149#discussion_r2016872082)
I can see I got confused with other tests that validate it but I see they call `migratewallet` instead (and then `get_wallet_rpc` - I can do a follow-up to clean it up later). Same for `assert_is_sqlite`, I'll remove it, since `migrate_and_get_rpc()` already calls it within. Thanks!
💬 pablomartin4btc commented on pull request "wallet, migration: Fix empty wallet crash":
(https://github.com/bitcoin/bitcoin/pull/32149#discussion_r2016874922)
Yup, much better! Cheers!
(https://github.com/bitcoin/bitcoin/pull/32149#discussion_r2016874922)
Yup, much better! Cheers!
🚀 ryanofsky merged a pull request: "test: Add functional test for bitcoin-chainstate"
(https://github.com/bitcoin/bitcoin/pull/32145)
(https://github.com/bitcoin/bitcoin/pull/32145)
💬 ryanofsky commented on pull request "test: avoid treating hash results as integers":
(https://github.com/bitcoin/bitcoin/pull/32050#issuecomment-2758417008)
re: https://github.com/bitcoin/bitcoin/pull/32050#issuecomment-2722365413
> I agree that displaying hash results as byte-reversed hex is an arbitrary decision, but one that stems from the Satoshi-era past and all Bitcoin protocol software that deals with user interface follows it (see also https://bitcoin.stackexchange.com/questions/116730/why-does-bitcoin-core-print-sha256-hashes-uint256-bytes-in-reverse-order).
I think my point was that it wasn't an arbitrary decision but an intentional
...
(https://github.com/bitcoin/bitcoin/pull/32050#issuecomment-2758417008)
re: https://github.com/bitcoin/bitcoin/pull/32050#issuecomment-2722365413
> I agree that displaying hash results as byte-reversed hex is an arbitrary decision, but one that stems from the Satoshi-era past and all Bitcoin protocol software that deals with user interface follows it (see also https://bitcoin.stackexchange.com/questions/116730/why-does-bitcoin-core-print-sha256-hashes-uint256-bytes-in-reverse-order).
I think my point was that it wasn't an arbitrary decision but an intentional
...
💬 TheCharlatan commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2016904181)
Missed this in my previous round, but is this not changing behaviour significantly? As far as I can tell we now just check that the target blockhash is reached at some point, while this check that is removed here only skips blocks up to the snapshot height and tries to complete validation with the rest once the snapshot base height is reached. What if the snapshot block is never reached? Does background validation then just continue on indefinitely?
I might be missing something, but the way
...
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2016904181)
Missed this in my previous round, but is this not changing behaviour significantly? As far as I can tell we now just check that the target blockhash is reached at some point, while this check that is removed here only skips blocks up to the snapshot height and tries to complete validation with the rest once the snapshot base height is reached. What if the snapshot block is never reached? Does background validation then just continue on indefinitely?
I might be missing something, but the way
...
💬 TheCharlatan commented on pull request "kernel: Flush in ChainstateManager destructor":
(https://github.com/bitcoin/bitcoin/pull/31382#issuecomment-2758442504)
Rebased fe7d09b5017b748fc71dab2a8bb186ff2c1624bb -> 1c299f70833188654b9cf3936d63d370856a408d ([chainman_flush_destructor_2](https://github.com/TheCharlatan/bitcoin/tree/chainman_flush_destructor_2) -> [chainman_flush_destructor_3](https://github.com/TheCharlatan/bitcoin/tree/chainman_flush_destructor_3), [compare](https://github.com/TheCharlatan/bitcoin/compare/chainman_flush_destructor_2..chainman_flush_destructor_3))
* Rebased to get the new bitcoin-chainstate functional test
(https://github.com/bitcoin/bitcoin/pull/31382#issuecomment-2758442504)
Rebased fe7d09b5017b748fc71dab2a8bb186ff2c1624bb -> 1c299f70833188654b9cf3936d63d370856a408d ([chainman_flush_destructor_2](https://github.com/TheCharlatan/bitcoin/tree/chainman_flush_destructor_2) -> [chainman_flush_destructor_3](https://github.com/TheCharlatan/bitcoin/tree/chainman_flush_destructor_3), [compare](https://github.com/TheCharlatan/bitcoin/compare/chainman_flush_destructor_2..chainman_flush_destructor_3))
* Rebased to get the new bitcoin-chainstate functional test
💬 fjahr commented on pull request "contrib: document asmap-tool commands more thoroughly":
(https://github.com/bitcoin/bitcoin/pull/32110#discussion_r2016886163)
nit: Well, the ones we make with kartograf are that large. If someone else makes one based on their personal BGP dump or they don't use all the sources in kartograf they may be smaller. But it's not a big deal, the point is that people understand that we want to reduce the size. I would have just left the specific number out. But I would say you leave as is unless you have to retouch.
(https://github.com/bitcoin/bitcoin/pull/32110#discussion_r2016886163)
nit: Well, the ones we make with kartograf are that large. If someone else makes one based on their personal BGP dump or they don't use all the sources in kartograf they may be smaller. But it's not a big deal, the point is that people understand that we want to reduce the size. I would have just left the specific number out. But I would say you leave as is unless you have to retouch.
🤔 fjahr reviewed a pull request: "contrib: document asmap-tool commands more thoroughly"
(https://github.com/bitcoin/bitcoin/pull/32110#pullrequestreview-2722087407)
Code review ACK 704573e016ac40e7cae00a84a9ba532a760e043e
My comments are not critical but I can quickly re-review if you decide to address them.
(https://github.com/bitcoin/bitcoin/pull/32110#pullrequestreview-2722087407)
Code review ACK 704573e016ac40e7cae00a84a9ba532a760e043e
My comments are not critical but I can quickly re-review if you decide to address them.
💬 fjahr commented on pull request "contrib: document asmap-tool commands more thoroughly":
(https://github.com/bitcoin/bitcoin/pull/32110#discussion_r2016920827)
nit: I find the placement of this example block a bit weird or I misunderstand it's purpose. Unless there is something I overlooked I think it might make more sense if it was right below the bullet list of the different output states, with a statement on the top saying something like "Part of an output file could look like this for example:". Right now, because the `--ignore-unassigned` part is right above it, I thought the block would demonstrate something about this flag but the block includes
...
(https://github.com/bitcoin/bitcoin/pull/32110#discussion_r2016920827)
nit: I find the placement of this example block a bit weird or I misunderstand it's purpose. Unless there is something I overlooked I think it might make more sense if it was right below the bullet list of the different output states, with a statement on the top saying something like "Part of an output file could look like this for example:". Right now, because the `--ignore-unassigned` part is right above it, I thought the block would demonstrate something about this flag but the block includes
...