💬 kevkevinpal commented on pull request "test: refactor: use built-in collection types for type hints (Python 3.9 / PEP 585)":
(https://github.com/bitcoin/bitcoin/pull/28725#issuecomment-1802851411)
running these two grep commands I got no results back which is good
`grep -nr -e "Dict\[" -e "List\[" -e "Set\[" -e "Type\[" -e "Tuple\[" -e "FrozenSet\[" ./test`
`grep -nr -e "Dict\[" -e "List\[" -e "Set\[" -e "Type\[" -e "Tuple\[" -e "FrozenSet\[" ./contrib`
and then running these and checking if there were anything we missed
`grep -nri "from typing import" ./contrib`
`grep -nri "from typing import" ./test`
but only found ones that we couldn't use builtin collection types for
do w
...
(https://github.com/bitcoin/bitcoin/pull/28725#issuecomment-1802851411)
running these two grep commands I got no results back which is good
`grep -nr -e "Dict\[" -e "List\[" -e "Set\[" -e "Type\[" -e "Tuple\[" -e "FrozenSet\[" ./test`
`grep -nr -e "Dict\[" -e "List\[" -e "Set\[" -e "Type\[" -e "Tuple\[" -e "FrozenSet\[" ./contrib`
and then running these and checking if there were anything we missed
`grep -nri "from typing import" ./contrib`
`grep -nri "from typing import" ./test`
but only found ones that we couldn't use builtin collection types for
do w
...
💬 ajtowns commented on pull request "rpc: Add script verification flags to getdeploymentinfo":
(https://github.com/bitcoin/bitcoin/pull/28806#issuecomment-1802885826)
> Ah, #10730
"When we last discussed making scripts debuggable" -- this and #28802 are prereqs for "bitcoin-util evalscript" which I'm poking at on https://github.com/ajtowns/bitcoin/commits/202309-evalscript . Real debugging seems better done with smarter external tools like https://github.com/dgpv/bsst though.
(https://github.com/bitcoin/bitcoin/pull/28806#issuecomment-1802885826)
> Ah, #10730
"When we last discussed making scripts debuggable" -- this and #28802 are prereqs for "bitcoin-util evalscript" which I'm poking at on https://github.com/ajtowns/bitcoin/commits/202309-evalscript . Real debugging seems better done with smarter external tools like https://github.com/dgpv/bsst though.
👍 theStack approved a pull request: "refactor: Miniminer package linearization followups"
(https://github.com/bitcoin/bitcoin/pull/28808#pullrequestreview-1721469632)
LGTM ACK b4b01d3fb42e7b688d97b75f57cfe18cfca6d943
(https://github.com/bitcoin/bitcoin/pull/28808#pullrequestreview-1721469632)
LGTM ACK b4b01d3fb42e7b688d97b75f57cfe18cfca6d943
💬 Sjors commented on pull request "Show transactions as not fully confirmed during background validation":
(https://github.com/bitcoin/bitcoin/pull/28616#discussion_r1387351444)
Interesting that you're getting that error when loading from a backup, but not when loading a regular wallet.
(https://github.com/bitcoin/bitcoin/pull/28616#discussion_r1387351444)
Interesting that you're getting that error when loading from a backup, but not when loading a regular wallet.
💬 Sjors commented on pull request "Show transactions as not fully confirmed during background validation":
(https://github.com/bitcoin/bitcoin/pull/28616#issuecomment-1802999311)
Rebased to see if the asserts added in #28546 catch anything here. (They don't in the test suite and manual testing).
(https://github.com/bitcoin/bitcoin/pull/28616#issuecomment-1802999311)
Rebased to see if the asserts added in #28546 catch anything here. (They don't in the test suite and manual testing).
💬 Sjors commented on pull request "Show transactions as not fully confirmed during background validation":
(https://github.com/bitcoin/bitcoin/pull/28616#discussion_r1387360904)
> I think wallet tests should be separate from consensus tests.
That makes sense.
(https://github.com/bitcoin/bitcoin/pull/28616#discussion_r1387360904)
> I think wallet tests should be separate from consensus tests.
That makes sense.
💬 Sjors commented on pull request "bugfix: throw an error if an invalid parameter is passed to getnetworkhashps RPC":
(https://github.com/bitcoin/bitcoin/pull/28554#issuecomment-1803011008)
Thanks for rebasing.
re-utACK 9ac114e5cd9d8ade3a1d9f3d76a08ff59a3f1658
(https://github.com/bitcoin/bitcoin/pull/28554#issuecomment-1803011008)
Thanks for rebasing.
re-utACK 9ac114e5cd9d8ade3a1d9f3d76a08ff59a3f1658
💬 Sjors commented on pull request "contrib: Add asmap-tool":
(https://github.com/bitcoin/bitcoin/pull/28793#issuecomment-1803013981)
I'm fine with adding another contrib script here. But we could also make a new repo under bitcoin-core. Perhaps the choice depends on whether this tool is mainly useful for users, developers or maintainers ([bitcoin-core/bitcoin-maintainer-tools](https://github.com/bitcoin-core/bitcoin-maintainer-tools))?
(https://github.com/bitcoin/bitcoin/pull/28793#issuecomment-1803013981)
I'm fine with adding another contrib script here. But we could also make a new repo under bitcoin-core. Perhaps the choice depends on whether this tool is mainly useful for users, developers or maintainers ([bitcoin-core/bitcoin-maintainer-tools](https://github.com/bitcoin-core/bitcoin-maintainer-tools))?
💬 Sjors commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#issuecomment-1803029813)
re-utACK b31d9bf1ae764421c8937dd126ad9e29845ebc5e
(https://github.com/bitcoin/bitcoin/pull/26728#issuecomment-1803029813)
re-utACK b31d9bf1ae764421c8937dd126ad9e29845ebc5e
💬 Sjors commented on pull request "Show transactions as not fully confirmed during background validation":
(https://github.com/bitcoin/bitcoin/pull/28616#discussion_r1387417011)
Added a wallet test file here: 59796c3926cb146229a2b3e942a5f6994f27d72a
It made me realise we should probably disallow loading a wallet with a birth date below the assume valid block. Though for manual testing it's handy.
(https://github.com/bitcoin/bitcoin/pull/28616#discussion_r1387417011)
Added a wallet test file here: 59796c3926cb146229a2b3e942a5f6994f27d72a
It made me realise we should probably disallow loading a wallet with a birth date below the assume valid block. Though for manual testing it's handy.
💬 Sjors commented on issue "test: Write assumeutxo tests":
(https://github.com/bitcoin/bitcoin/issues/28648#issuecomment-1803074127)
I created a wallet test file as part of #28616. Feel free to lift that commit out and make a fresh pull request. Right now it doesn't actually test the functionality that's introduced in 28616, though I might expand it.
(https://github.com/bitcoin/bitcoin/issues/28648#issuecomment-1803074127)
I created a wallet test file as part of #28616. Feel free to lift that commit out and make a fresh pull request. Right now it doesn't actually test the functionality that's introduced in 28616, though I might expand it.
💬 romanz commented on pull request "rpc: keep `.cookie` file if it was not generated":
(https://github.com/bitcoin/bitcoin/pull/28784#issuecomment-1803120456)
Thanks for the review!
Squashed and force-pushed d95dde9441fb791046394ed3784a840a54ef2ab9.
(https://github.com/bitcoin/bitcoin/pull/28784#issuecomment-1803120456)
Thanks for the review!
Squashed and force-pushed d95dde9441fb791046394ed3784a840a54ef2ab9.
💬 ajtowns commented on pull request "Use LE hex-encoded representations in script ASM for pushed values <= 4 bytes":
(https://github.com/bitcoin/bitcoin/pull/28824#discussion_r1387462425)
This seems much harder to understand to me -- you need to convert it to `0x07a120` then convert that to decimal to get back to the nice round number block height? In particular mistaking it for `0x20a107` (2,138,375) would be particularly misleading.
(https://github.com/bitcoin/bitcoin/pull/28824#discussion_r1387462425)
This seems much harder to understand to me -- you need to convert it to `0x07a120` then convert that to decimal to get back to the nice round number block height? In particular mistaking it for `0x20a107` (2,138,375) would be particularly misleading.
💬 ajtowns commented on pull request "Use LE hex-encoded representations in script ASM for pushed values <= 4 bytes":
(https://github.com/bitcoin/bitcoin/pull/28824#discussion_r1387463136)
This also seems confusing, particularly if it appears in something like `DUP SIZE 20 EQUALVERIFY HASH160 <x> EQUAL` -- whoops, that's actually saying the input should be 32 bytes, not 20 bytes.
(https://github.com/bitcoin/bitcoin/pull/28824#discussion_r1387463136)
This also seems confusing, particularly if it appears in something like `DUP SIZE 20 EQUALVERIFY HASH160 <x> EQUAL` -- whoops, that's actually saying the input should be 32 bytes, not 20 bytes.
💬 Sjors commented on pull request "Show transactions as not fully confirmed during background validation":
(https://github.com/bitcoin/bitcoin/pull/28616#discussion_r1387466566)
I'm still trying to wrap my head around the logic in `CWallet::AttachChain` which is what triggers the error when loading a backup, but somehow doesn't trigger it when loading an existing wallet.
(https://github.com/bitcoin/bitcoin/pull/28616#discussion_r1387466566)
I'm still trying to wrap my head around the logic in `CWallet::AttachChain` which is what triggers the error when loading a backup, but somehow doesn't trigger it when loading an existing wallet.
💬 Sjors commented on pull request "Show transactions as not fully confirmed during background validation":
(https://github.com/bitcoin/bitcoin/pull/28616#discussion_r1387468948)
Oh I see, the test creates the backup _before_ the snapshot height.
(https://github.com/bitcoin/bitcoin/pull/28616#discussion_r1387468948)
Oh I see, the test creates the backup _before_ the snapshot height.
💬 Sjors commented on pull request "Show transactions as not fully confirmed during background validation":
(https://github.com/bitcoin/bitcoin/pull/28616#discussion_r1387470744)
As for the behavior that this PR introduces, ideally there would a `stopatheight` RPC, but the test could also - as it does now - use `-stopatheight` and restart the node. Since it won't automatically connect to the other test nodes, we can inspect the result of `getwallettransactions` and see if `assumed` is set correctly.
I'll work on that and make it a separate commit, so it's a bit easier to extract the test scaffold if this PR doesn't make it.
(https://github.com/bitcoin/bitcoin/pull/28616#discussion_r1387470744)
As for the behavior that this PR introduces, ideally there would a `stopatheight` RPC, but the test could also - as it does now - use `-stopatheight` and restart the node. Since it won't automatically connect to the other test nodes, we can inspect the result of `getwallettransactions` and see if `assumed` is set correctly.
I'll work on that and make it a separate commit, so it's a bit easier to extract the test scaffold if this PR doesn't make it.
📝 momodaka opened a pull request: "[doc]: uppercase title for "Assumeutxo""
(https://github.com/bitcoin/bitcoin/pull/28827)
uppercase title from `# assumeutxo` to `# Assumeutxo`
(https://github.com/bitcoin/bitcoin/pull/28827)
uppercase title from `# assumeutxo` to `# Assumeutxo`
✅ momodaka closed a pull request: "[doc]: uppercase title for "Assumeutxo""
(https://github.com/bitcoin/bitcoin/pull/28827)
(https://github.com/bitcoin/bitcoin/pull/28827)
📝 momodaka opened a pull request: "[doc]: uppercase title for "Assumeutxo“"
(https://github.com/bitcoin/bitcoin/pull/28828)
uppercase title from `# assumeutxo` to `# Assumeutxo`
(https://github.com/bitcoin/bitcoin/pull/28828)
uppercase title from `# assumeutxo` to `# Assumeutxo`