💬 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`
💬 ajtowns commented on pull request "rpc: Add script verification flags to getdeploymentinfo":
(https://github.com/bitcoin/bitcoin/pull/28806#discussion_r1387504207)
"Pay to script hash" is the title of BIP16, and we use both "pay-to-script-hash" and "P2SH" as descriptions of the `sh()` descriptor in doc/descriptors.md, so I don't think this is confusing or needs any changes.
(https://github.com/bitcoin/bitcoin/pull/28806#discussion_r1387504207)
"Pay to script hash" is the title of BIP16, and we use both "pay-to-script-hash" and "P2SH" as descriptions of the `sh()` descriptor in doc/descriptors.md, so I don't think this is confusing or needs any changes.
👍 kristapsk approved a pull request: "rpc: keep `.cookie` file if it was not generated"
(https://github.com/bitcoin/bitcoin/pull/28784#pullrequestreview-1721753043)
re-ACK d95dde9441fb791046394ed3784a840a54ef2ab9
(https://github.com/bitcoin/bitcoin/pull/28784#pullrequestreview-1721753043)
re-ACK d95dde9441fb791046394ed3784a840a54ef2ab9
💬 maaku commented on pull request "snapshots: don't core dump when running -checkblockindex after `loadtxoutset`":
(https://github.com/bitcoin/bitcoin/pull/28791#issuecomment-1803233389)
@luke-jr That would require storing an integer for every historical block. That would only add a megabyte or so to the snapshot size, so it would be quite doable. But it seems kinda silly when the field isn't used (except for this assert), and nothing else about the prior block data is validated.
(https://github.com/bitcoin/bitcoin/pull/28791#issuecomment-1803233389)
@luke-jr That would require storing an integer for every historical block. That would only add a megabyte or so to the snapshot size, so it would be quite doable. But it seems kinda silly when the field isn't used (except for this assert), and nothing else about the prior block data is validated.
💬 maaku commented on pull request "snapshots: don't core dump when running -checkblockindex after `loadtxoutset`":
(https://github.com/bitcoin/bitcoin/pull/28791#discussion_r1387544004)
Yes, the entire assert block is wrong. When a snapshot is loaded, the `pindex->nTx` value is set to 1. So I'm not really sure what these exceptions would be allowing through.
But since this issue affects the v26 release, I've tried to keep the code delta as small and minimally impactful as possible.
(https://github.com/bitcoin/bitcoin/pull/28791#discussion_r1387544004)
Yes, the entire assert block is wrong. When a snapshot is loaded, the `pindex->nTx` value is set to 1. So I'm not really sure what these exceptions would be allowing through.
But since this issue affects the v26 release, I've tried to keep the code delta as small and minimally impactful as possible.
💬 ajtowns commented on pull request "rpc: Add script verification flags to getdeploymentinfo":
(https://github.com/bitcoin/bitcoin/pull/28806#discussion_r1387582295)
Huh, had thought about that previously and didn't like it. Reconsidering it, now I do. :shrug:
(https://github.com/bitcoin/bitcoin/pull/28806#discussion_r1387582295)
Huh, had thought about that previously and didn't like it. Reconsidering it, now I do. :shrug:
💬 ajtowns commented on pull request "rpc: Add script verification flags to getdeploymentinfo":
(https://github.com/bitcoin/bitcoin/pull/28806#discussion_r1387584353)
Numbers aren't a standard (they'll readily change between releases; also true of the names) but they also aren't particularly useful as documentation, so don't think it's sensible to export them here.
(https://github.com/bitcoin/bitcoin/pull/28806#discussion_r1387584353)
Numbers aren't a standard (they'll readily change between releases; also true of the names) but they also aren't particularly useful as documentation, so don't think it's sensible to export them here.
💬 naumenkogs commented on pull request "net: Add new permission `forceinbound` to evict a random unprotected connection if all slots are otherwise full":
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1387597063)
c5e7e6563201965e88e07aa8da8f26e17fc1b4db
"unprotected" here is sloppy. I'd just drop the comment.
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1387597063)
c5e7e6563201965e88e07aa8da8f26e17fc1b4db
"unprotected" here is sloppy. I'd just drop the comment.
💬 naumenkogs commented on pull request "net: Add new permission `forceinbound` to evict a random unprotected connection if all slots are otherwise full":
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1387601029)
c5e7e6563201965e88e07aa8da8f26e17fc1b4db
"among inbound no-noban connections" or something (i hate `no-noban` but not sure what's better)
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1387601029)
c5e7e6563201965e88e07aa8da8f26e17fc1b4db
"among inbound no-noban connections" or something (i hate `no-noban` but not sure what's better)
💬 naumenkogs commented on pull request "net: Add new permission `forceinbound` to evict a random unprotected connection if all slots are otherwise full":
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1387608833)
311902f2cf94ee0e11ed34a1373db42dbc218b20
No need to use legacy naming patterns for new variables :)
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1387608833)
311902f2cf94ee0e11ed34a1373db42dbc218b20
No need to use legacy naming patterns for new variables :)