💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1959105072)
After thinking about this a bit more and doing some changes to the code, I decided to:
1. Open a separate PR to remove the manual ref counting on `CNode` and replace it with `shared_ptr` and change `CConnman::m_nodes` from `vector<CNode*>` to `unordered_set<shared_ptr<CNode>>`. That PR would be independent from this one.
2. In this PR, change the communication between `SockMan` and `CConman` to be based on pointer to `CNode` instead of node id. Similarly to https://github.com/bitcoin/bitco
...
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1959105072)
After thinking about this a bit more and doing some changes to the code, I decided to:
1. Open a separate PR to remove the manual ref counting on `CNode` and replace it with `shared_ptr` and change `CConnman::m_nodes` from `vector<CNode*>` to `unordered_set<shared_ptr<CNode>>`. That PR would be independent from this one.
2. In this PR, change the communication between `SockMan` and `CConman` to be based on pointer to `CNode` instead of node id. Similarly to https://github.com/bitcoin/bitco
...
💬 maflcko commented on issue "Wallet passpharse":
(https://github.com/bitcoin/bitcoin/issues/31852#issuecomment-2664725982)
Absent anyone else being able to reproduce, you could of course also try to reproduce on Linux or macOS. This could rule out an issue specific to your Windows setup.
(https://github.com/bitcoin/bitcoin/issues/31852#issuecomment-2664725982)
Absent anyone else being able to reproduce, you could of course also try to reproduce on Linux or macOS. This could rule out an issue specific to your Windows setup.
💬 maflcko commented on pull request "wip: Split fuzz binary (take 2)":
(https://github.com/bitcoin/bitcoin/pull/30882#issuecomment-2664740634)
It is now using FI-light, see https://introspector.oss-fuzz.com/project-profile?project=bitcoin-core and https://github.com/google/oss-fuzz/pull/12983. Though, I am not sure about the takeaways.
(https://github.com/bitcoin/bitcoin/pull/30882#issuecomment-2664740634)
It is now using FI-light, see https://introspector.oss-fuzz.com/project-profile?project=bitcoin-core and https://github.com/google/oss-fuzz/pull/12983. Though, I am not sure about the takeaways.
💬 maflcko commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#issuecomment-2664769789)
> @DrahtBot is drunk again? Asking me for a review directly after I give a (partial) review.
It will never be possible to be 100% accurate in a heuristic. If it bugs you, you may be able to work around it by leaving a fresh review comment with a `(concept/approach) ack (commit_id), but I skipped over some commits` (or similar), according to https://github.com/maflcko/DrahtBot/blob/1cce096df7cb4d89d34bcdbed09b20a38fe2a344/webhook_features/src/features/summary_comment.rs#L318-L320. You may also
...
(https://github.com/bitcoin/bitcoin/pull/31622#issuecomment-2664769789)
> @DrahtBot is drunk again? Asking me for a review directly after I give a (partial) review.
It will never be possible to be 100% accurate in a heuristic. If it bugs you, you may be able to work around it by leaving a fresh review comment with a `(concept/approach) ack (commit_id), but I skipped over some commits` (or similar), according to https://github.com/maflcko/DrahtBot/blob/1cce096df7cb4d89d34bcdbed09b20a38fe2a344/webhook_features/src/features/summary_comment.rs#L318-L320. You may also
...
💬 maflcko commented on pull request "refactor: remove redundant `for` constructs":
(https://github.com/bitcoin/bitcoin/pull/31891#issuecomment-2664778729)
Tend toward NACK. This just makes the test harder to edit to adjust the counts for the amounts to be different. Either way, it is harmless and fine to keep as-is.
(https://github.com/bitcoin/bitcoin/pull/31891#issuecomment-2664778729)
Tend toward NACK. This just makes the test harder to edit to adjust the counts for the amounts to be different. Either way, it is harmless and fine to keep as-is.
🤔 zaidmstrr reviewed a pull request: "fuzz: split `coinselection` harness"
(https://github.com/bitcoin/bitcoin/pull/31870#pullrequestreview-2622790831)
Tested ACK [9b293d0](https://github.com/bitcoin/bitcoin/pull/31870/commits/9b293d031c941dc6af0c4274e43388c57c702047)
The idea of fuzzing each algorithm independently seems good. I manually reviewed the new code changes with the previous ones to check for any missing statements or logical errors. I also tested the code by manually inserting assert statements in between to check whether the desired behaviour was occurring or not.
(https://github.com/bitcoin/bitcoin/pull/31870#pullrequestreview-2622790831)
Tested ACK [9b293d0](https://github.com/bitcoin/bitcoin/pull/31870/commits/9b293d031c941dc6af0c4274e43388c57c702047)
The idea of fuzzing each algorithm independently seems good. I manually reviewed the new code changes with the previous ones to check for any missing statements or logical errors. I also tested the code by manually inserting assert statements in between to check whether the desired behaviour was occurring or not.
👍 rkrux approved a pull request: "wallet: fix crash on double block disconnection"
(https://github.com/bitcoin/bitcoin/pull/31757#pullrequestreview-2622808152)
I have checked that a coinbase transaction is set to "inactive + abandoned" here on first block disconnect inside the `AddToWallet` function: https://github.com/bitcoin/bitcoin/blob/master/src/wallet/wallet.cpp#L1132
And on an unwanted second block disconnection, the same state needs to be passed so that the assertion on line 1114 is satisfied if it's not a new transaction and with the same state variant: https://github.com/bitcoin/bitcoin/blob/master/src/wallet/wallet.cpp#L1114
I cherry-p
...
(https://github.com/bitcoin/bitcoin/pull/31757#pullrequestreview-2622808152)
I have checked that a coinbase transaction is set to "inactive + abandoned" here on first block disconnect inside the `AddToWallet` function: https://github.com/bitcoin/bitcoin/blob/master/src/wallet/wallet.cpp#L1132
And on an unwanted second block disconnection, the same state needs to be passed so that the assertion on line 1114 is satisfied if it's not a new transaction and with the same state variant: https://github.com/bitcoin/bitcoin/blob/master/src/wallet/wallet.cpp#L1114
I cherry-p
...
💬 rkrux commented on pull request "wallet: fix crash on double block disconnection":
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1959294653)
The same function seems to be used in the first block disconnection as well:
https://github.com/bitcoin/bitcoin/blob/master/src/wallet/wallet.cpp#L1129
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1959294653)
The same function seems to be used in the first block disconnection as well:
https://github.com/bitcoin/bitcoin/blob/master/src/wallet/wallet.cpp#L1129
👍 maflcko approved a pull request: "wallet: Disable creating and loading legacy wallets"
(https://github.com/bitcoin/bitcoin/pull/31250#pullrequestreview-2622685585)
Needs rebase.
I left some nits, feel free to ignore.
I skipped over the large commit and the last two.
Otherwise:
review ACK 82fc65c9ab53c416935b5eb20be10fe69ded8b88 👨
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGr
...
(https://github.com/bitcoin/bitcoin/pull/31250#pullrequestreview-2622685585)
Needs rebase.
I left some nits, feel free to ignore.
I skipped over the large commit and the last two.
Otherwise:
review ACK 82fc65c9ab53c416935b5eb20be10fe69ded8b88 👨
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGr
...
💬 maflcko commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1959218136)
nit in 321b1cf376865ecc720bbd32357553d508704a35: Should remove the redundant bool now as well?
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1959218136)
nit in 321b1cf376865ecc720bbd32357553d508704a35: Should remove the redundant bool now as well?
💬 maflcko commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1959226159)
nit in https://github.com/bitcoin/bitcoin/commit/321b1cf376865ecc720bbd32357553d508704a35: Should the test be kept and be done with a descriptor wallet instead?
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1959226159)
nit in https://github.com/bitcoin/bitcoin/commit/321b1cf376865ecc720bbd32357553d508704a35: Should the test be kept and be done with a descriptor wallet instead?
💬 maflcko commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1959293746)
nit in f19592a06059b44d55838bf2f8c63fde2917ee83: Could extend the error msg? Something like:
`"Error: Dumpfile specifies an unsupported database format (%s). Only sqlite database dumps are supported."`?
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1959293746)
nit in f19592a06059b44d55838bf2f8c63fde2917ee83: Could extend the error msg? Something like:
`"Error: Dumpfile specifies an unsupported database format (%s). Only sqlite database dumps are supported."`?
💬 maflcko commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1959251350)
q in bb6a15173eef114c60e620b431a4306f8de213d0: I don't understand this commit. It simply states something and removes code, but I don't see the connection. Maybe a note could help to explain why and when this is needed?
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1959251350)
q in bb6a15173eef114c60e620b431a4306f8de213d0: I don't understand this commit. It simply states something and removes code, but I don't see the connection. Maybe a note could help to explain why and when this is needed?
💬 maflcko commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1959259156)
nit in 0b84b25e92d382ace40f8b52b80925b3fe76e5f3: Forgot to remove unused `legacy_nodes`?
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1959259156)
nit in 0b84b25e92d382ace40f8b52b80925b3fe76e5f3: Forgot to remove unused `legacy_nodes`?
💬 maflcko commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1959279697)
nit in 69eb2c4d85098acbad6b89a09fae4ca84e81fff5: Why set to true redundantly, when no other test does this and when `skip_if_no_wallet` already does it?
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1959279697)
nit in 69eb2c4d85098acbad6b89a09fae4ca84e81fff5: Why set to true redundantly, when no other test does this and when `skip_if_no_wallet` already does it?
💬 maflcko commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1959304120)
eb56556ca4a9104e22cec70698ffceb22719894c: This removes sqlite, but it would be better to remove bdb? Also the commit message needs fixup.
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1959304120)
eb56556ca4a9104e22cec70698ffceb22719894c: This removes sqlite, but it would be better to remove bdb? Also the commit message needs fixup.
💬 rkrux commented on pull request "wallet: fix crash on double block disconnection":
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1959342651)
```diff
- info.prev_hash = tip->phashBlock;
+ info.prev_hash = tip->pprev->phashBlock;
```
The hash of the previous block needs to be added here instead of the hash of the same block. I ran the test with this and it works.
Reference: `MakeBlockInfo()` here https://github.com/bitcoin/bitcoin/blob/9da0820ec55e136d5213bf5bb90c36499debc034/src/kernel/chain.cpp#L18
The `pprev` presence check can be avoided because the test is using `TestChain100Setup` that has 100 blocks mined already lea
...
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1959342651)
```diff
- info.prev_hash = tip->phashBlock;
+ info.prev_hash = tip->pprev->phashBlock;
```
The hash of the previous block needs to be added here instead of the hash of the same block. I ran the test with this and it works.
Reference: `MakeBlockInfo()` here https://github.com/bitcoin/bitcoin/blob/9da0820ec55e136d5213bf5bb90c36499debc034/src/kernel/chain.cpp#L18
The `pprev` presence check can be avoided because the test is using `TestChain100Setup` that has 100 blocks mined already lea
...
💬 rkrux commented on pull request "wallet: fix crash on double block disconnection":
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1959355949)
> // to by-pass the wallet birth time.
Can you explain this a bit more? I ran the test w/o it and it works.
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1959355949)
> // to by-pass the wallet birth time.
Can you explain this a bit more? I ran the test w/o it and it works.
💬 dergoegge commented on pull request "consensus: Remove checkpoints (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31649#issuecomment-2665081851)
> I think this should be merged after v29.x branch off so that it can be tested in master for several months before being released in v30.0
It's obviously fine to wait until v30.0 but I don't see why we should? If things on master get additional testing, then we've been testing the headers pre-sync for ~2.5 years now. I don't think this needs anymore testing or what exactly are the expectations here, i.e. what tests are we missing?
(https://github.com/bitcoin/bitcoin/pull/31649#issuecomment-2665081851)
> I think this should be merged after v29.x branch off so that it can be tested in master for several months before being released in v30.0
It's obviously fine to wait until v30.0 but I don't see why we should? If things on master get additional testing, then we've been testing the headers pre-sync for ~2.5 years now. I don't think this needs anymore testing or what exactly are the expectations here, i.e. what tests are we missing?
👍 TheCharlatan approved a pull request: "cmake: add optional source files to bitcoin_crypto and crc32c directly"
(https://github.com/bitcoin/bitcoin/pull/31268#pullrequestreview-2622958345)
ACK 9cf746d6631739df9c9f80accd5812b319efcfec
(https://github.com/bitcoin/bitcoin/pull/31268#pullrequestreview-2622958345)
ACK 9cf746d6631739df9c9f80accd5812b319efcfec