💬 mzumsande commented on pull request "p2p: Advance pindexLastCommonBlock early after connecting blocks":
(https://github.com/bitcoin/bitcoin/pull/32180#issuecomment-2836382102)
[c69ee2d](https://github.com/bitcoin/bitcoin/commit/c69ee2d5b93296d3008d6978182b2bc29bbeb457) to [b7ff6a6](https://github.com/bitcoin/bitcoin/commit/b7ff6a611a220e9380f6cd6428f1d3483c8d566f): addressed feedback
> Is it expected to pass sometimes?
It wasn't - but I have tracked this down now. It's a bit involved, I hope the following explanation makes sense:
The initial situation is that there are 2 remaining connected peers, the node has just synced blocks up to height 500, but pindex
...
(https://github.com/bitcoin/bitcoin/pull/32180#issuecomment-2836382102)
[c69ee2d](https://github.com/bitcoin/bitcoin/commit/c69ee2d5b93296d3008d6978182b2bc29bbeb457) to [b7ff6a6](https://github.com/bitcoin/bitcoin/commit/b7ff6a611a220e9380f6cd6428f1d3483c8d566f): addressed feedback
> Is it expected to pass sometimes?
It wasn't - but I have tracked this down now. It's a bit involved, I hope the following explanation makes sense:
The initial situation is that there are 2 remaining connected peers, the node has just synced blocks up to height 500, but pindex
...
💬 mzumsande commented on pull request "p2p: Advance pindexLastCommonBlock early after connecting blocks":
(https://github.com/bitcoin/bitcoin/pull/32180#discussion_r2064454694)
done
(https://github.com/bitcoin/bitcoin/pull/32180#discussion_r2064454694)
done
💬 mzumsande commented on pull request "p2p: Advance pindexLastCommonBlock early after connecting blocks":
(https://github.com/bitcoin/bitcoin/pull/32180#discussion_r2064455709)
changed to use one var for each index, plus one array.
(https://github.com/bitcoin/bitcoin/pull/32180#discussion_r2064455709)
changed to use one var for each index, plus one array.
💬 mzumsande commented on pull request "p2p: Advance pindexLastCommonBlock early after connecting blocks":
(https://github.com/bitcoin/bitcoin/pull/32180#discussion_r2064457862)
I'd say probably not becasue, `pindexLastCommonBlock` isn't really used for anything but assigning blocks during IBD.
If we will never need any blocks from this peer (until it sends us more headers, but then `pindexLastCommonBlock` will get updated. I'll also ignore weird `invalidateblock` scenarios here), I don't see what benefits updating would have.
(https://github.com/bitcoin/bitcoin/pull/32180#discussion_r2064457862)
I'd say probably not becasue, `pindexLastCommonBlock` isn't really used for anything but assigning blocks during IBD.
If we will never need any blocks from this peer (until it sends us more headers, but then `pindexLastCommonBlock` will get updated. I'll also ignore weird `invalidateblock` scenarios here), I don't see what benefits updating would have.
💬 davidgumberg commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#issuecomment-2836389658)
> Backwards compatibility is needed for wallet.dat
Bitcoin Core will still be able to migrate wallets that are in the legacy format to the new format. See the `migratewallet` rpc command: https://bitcoincore.org/en/doc/29.0.0/rpc/wallet/migratewallet/ and the legacy wallet deprecation tracking issue (https://github.com/bitcoin/bitcoin/issues/20160) for more context.
(https://github.com/bitcoin/bitcoin/pull/31250#issuecomment-2836389658)
> Backwards compatibility is needed for wallet.dat
Bitcoin Core will still be able to migrate wallets that are in the legacy format to the new format. See the `migratewallet` rpc command: https://bitcoincore.org/en/doc/29.0.0/rpc/wallet/migratewallet/ and the legacy wallet deprecation tracking issue (https://github.com/bitcoin/bitcoin/issues/20160) for more context.
💬 achow101 commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2064466039)
Probably, but I'm going to leave that for a followup, this diff is large enough already.
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2064466039)
Probably, but I'm going to leave that for a followup, this diff is large enough already.
💬 achow101 commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2064475635)
Descriptor wallets can't use non-HD keys in the "keypool", so it doesn't make sense to keep this for such wallets.
All migrated wallets get new descriptors anyways for new addresses. The keypool is entirely dumped.
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2064475635)
Descriptor wallets can't use non-HD keys in the "keypool", so it doesn't make sense to keep this for such wallets.
All migrated wallets get new descriptors anyways for new addresses. The keypool is entirely dumped.
💬 petertodd commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#discussion_r2064478495)
Added comment.
(https://github.com/bitcoin/bitcoin/pull/32359#discussion_r2064478495)
Added comment.
💬 achow101 commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2064483504)
`flush()` is inherited from `ChainClient`, which defines it as pure virtual, so it needs to have an implementation here even if it does nothing.
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2064483504)
`flush()` is inherited from `ChainClient`, which defines it as pure virtual, so it needs to have an implementation here even if it does nothing.
💬 petertodd commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#discussion_r2064485063)
I've applied that patch as-is.
(https://github.com/bitcoin/bitcoin/pull/32359#discussion_r2064485063)
I've applied that patch as-is.
📝 Retropex opened a pull request: "Catch nocive arbitrary data. (missing code for #32359)"
(https://github.com/bitcoin/bitcoin/pull/32368)
Some people in the discussion of the PR #32359 are suggesting that `OP_RETURN` should be used instead of `OP_IF`, `OP_FALSE` data injections.
IMO, PR #32359 should never be merged, as it would encourage the storage of arbitrary data on Bitcoin. But if the PR is still merged, then this code should be added, because [large data insertions have no incentive](https://bitcoin.stackexchange.com/a/122322) to move to `OP_RETURN`.
Some people in the discussion of the PR #32359 are suggesting that `OP_RETURN` should be used instead of `OP_IF`, `OP_FALSE` data injections.
IMO, PR #32359 should never be merged, as it would encourage the storage of arbitrary data on Bitcoin. But if the PR is still merged, then this code should be added, because [large data insertions have no incentive](https://bitcoin.stackexchange.com/a/122322) to move to `OP_RETURN`.
![image](https://github.com/user-attachments/assets/32005c35-9c38-42
...
💬 petertodd commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#discussion_r2064490005)
So we actually test the OP_RETURN followed by non-push in `src/test/transaction_tests.cpp`. But it wouldn't hurt to test it separately in terms of mempool-acceptance.
(https://github.com/bitcoin/bitcoin/pull/32359#discussion_r2064490005)
So we actually test the OP_RETURN followed by non-push in `src/test/transaction_tests.cpp`. But it wouldn't hurt to test it separately in terms of mempool-acceptance.
💬 petertodd commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#discussion_r2064504791)
I included this patch and @darosior's patch verbatim.
(https://github.com/bitcoin/bitcoin/pull/32359#discussion_r2064504791)
I included this patch and @darosior's patch verbatim.
💬 achow101 commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2064505718)
Indeed, removed.
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2064505718)
Indeed, removed.
💬 achow101 commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2064508912)
Apparently since this PR was opened, descriptor specific tests were added to those files. I've removed the legacy ones from them and added back to the `CMakeLists.txt`.
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2064508912)
Apparently since this PR was opened, descriptor specific tests were added to those files. I've removed the legacy ones from them and added back to the `CMakeLists.txt`.
💬 petertodd commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#discussion_r2064510978)
Fixed.
(https://github.com/bitcoin/bitcoin/pull/32359#discussion_r2064510978)
Fixed.
💬 achow101 commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2064511069)
Removed
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2064511069)
Removed
💬 achow101 commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2064514419)
Removed from the help text.
I've cleaned up some of the legacy wallet mentions that are no longer relevant. Some are still relevant, and others are less obviously legacy wallet only things so I will leave those for a followup.
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2064514419)
Removed from the help text.
I've cleaned up some of the legacy wallet mentions that are no longer relevant. Some are still relevant, and others are less obviously legacy wallet only things so I will leave those for a followup.
💬 achow101 commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2064514946)
Done
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2064514946)
Done
💬 achow101 commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2064515535)
Removed
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2064515535)
Removed