🤔 mzumsande reviewed a pull request: "test: Add missing wait for version to be sent in add_outbound_p2p_connection"
(https://github.com/bitcoin/bitcoin/pull/28822#pullrequestreview-1721365213)
ACK faa2ad88bc01bd434ce19fde19bcc5c78431702f

💬 TheCharlatan commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#issuecomment-1802781216)
Updated 3e7595b11bdad260efb39adc42677ed0beae186d -> 2be5e799ba623f969f5ffc59667a5bc6799df290 ([simplifyMemPoolInteractions_11](https://github.com/TheCharlatan/bitcoin/tree/simplifyMemPoolInteractions_11) -> [simplifyMemPoolInteractions_12](https://github.com/TheCharlatan/bitcoin/tree/simplifyMemPoolInteractions_12), [compare](https://github.com/TheCharlatan/bitcoin/compare/simplifyMemPoolInteractions_11..simplifyMemPoolInteractions_12))

* Addressed @glozow's [comment](https://github.com/bitc
...

💬 mzumsande commented on pull request "test: Make existing functional tests compatible with --v2transport":
(https://github.com/bitcoin/bitcoin/pull/28805#issuecomment-1802811199)
I changed the first commit to save the default behavior (global `--v2transport`) in the TestNode init and add an entry to `args`, and then deciding in `TestNode::start()` (which is called for both the initial startup and for restarts) based on this and possible `extra_args` whether to actually use v2 or not.

💬 vostrnad commented on pull request "script: add description for the functionality of each opcode":
(https://github.com/bitcoin/bitcoin/pull/27109#issuecomment-1802827804)
For anyone intersted in documenting opcodes, there is now https://opcodeexplained.com/ ([GitHub repo here](https://github.com/thunderbiscuit/opcode-explained)).

💬 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
...

💬 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.

👍 theStack approved a pull request: "refactor: Miniminer package linearization followups"
(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.

💬 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).

💬 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.

💬 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

💬 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))?

💬 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

💬 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.

💬 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.

💬 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.

💬 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.

💬 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.

💬 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.

💬 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.