💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1633714910)
Directly checked cluster size instead.

💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1633714959)
This should only be called when not `test_accept`, right?

💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1633714993)
removed the static cast for `m_conflicting_fees` since that's the same type as `m_total_modified_fees`, but left the other static casts in place since explicit is better than implicit

💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1633715042)
yep, rebased + removed

💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1633715153)
great, added a little commentary on top and repeated this for 2nd package

💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1633715573)
sure, taken

💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1633715847)
adapted more of the test to use this value

💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1633715933)
taken

💬 mzumsande commented on pull request "validation: Make ReplayBlocks interruptible":
(https://github.com/bitcoin/bitcoin/pull/30155#discussion_r1633721111)
> I would definitely be curious to see the other approach. Not just because it might be better at detecting inconsistent states like Luke is suggesting, but also because it seems like it would make the code more clear.

See https://github.com/mzumsande/bitcoin/commit/db8232d47068bba9ae1bb8135ef456be731e5ca5 - because of the inheritance the bool `interrupted_replay` needed to be added to quite a few spots, which I didn't love.

> I think it would be pretty hard to look at the code after this
...

💬 maflcko commented on pull request "test: add coverage for errors for `combinerawtransaction`":
(https://github.com/bitcoin/bitcoin/pull/30264#issuecomment-2159123696)
lgtm ACK ab98e6fd03970d6b5a593674c84e762a47b90ea6

👍 ryanofsky approved a pull request: "Support self-hosted Cirrus workers on forks"
(https://github.com/bitcoin/bitcoin/pull/29274#pullrequestreview-2108566302)
Code review ACK 65abc0b5b54edb06cc3bf584691beddbeb802ddc with caveat that I don't know enough about cirrus and github configuration to be very confident in the changes. But they do make sense and appear to do what's described.

💬 ryanofsky commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1633712140)
In commit "ci: test-each-commit merge base optional" (65abc0b5b54edb06cc3bf584691beddbeb802ddc)

Maybe add comment like "MERGE_BASE can be empty due to limited fetch-depth". Otherwise I don't think it's clear why git wouldn't be able to find the merge commit.

💬 ryanofsky commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1633739503)
In commit "ci: skip Github CI on branch pushes for forks" (1a06d80e7bb113b9588efc113262bdd20b60efd4)

I didn't really understand this comment until I read the PR description. Might be clearer as "Disable CI on branch pushes to forks. It will still run for pull requests. This prevents CI from running twice for typical pull request workflows."

Also, I'm not really sure how this change relates to the rest of the PR. It seems like it could be unrelated?

💬 ryanofsky commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1633717315)
In commit "ci: forks can opt-out of CI branch push" (8fbf7449af130126890932986a4d9c0446c2f9c8)

I don't really understand this comment. Maybe it could say why the jobs should not be marked as skipped, or maybe say how they should be shown instead of how they should not be shown.

💬 ryanofsky commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1633717668)
In commit "ci: forks can opt-out of CI branch push" (8fbf7449af130126890932986a4d9c0446c2f9c8)

What is $NO_BRANCH? Is it a custom variable like $NO_ARM added later? Could it be documented like NO_ARM is?

💬 maflcko commented on pull request "ci: move ASan job to GitHub Actions from Cirrus CI":
(https://github.com/bitcoin/bitcoin/pull/30193#discussion_r1633752828)
Should remove the `noble` type completely in this file

💬 maflcko commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#issuecomment-2159234188)
I could reproduce once outside the CI env on a 24.04 Ubuntu vm:

```
./configure CC='clang-18 -ftrivial-auto-var-init=pattern' CXX='clang++-18 -ftrivial-auto-var-init=pattern' --with-sanitizers=float-divide-by-zero,integer,undefined --enable-usdt --enable-zmq --with-incompatible-bdb --with-gui=qt5 && make

while ( UBSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1" ./test/functional/test_runner.py -j 22 --timeout-fact
...

📝 achow101 opened a pull request: "gui: Migrate legacy wallets that are not loaded"
(https://github.com/bitcoin-core/gui/pull/824)
Currently the Migrate Wallet menu item can only be used to migrate the currently loaded wallet. This is not suitable for the future when legacy wallets can no longer be loaded at all, but should still be able to be migrated. This PR changes that menu item into a menu list like Open Wallet and lets users migrate any legacy wallet in their wallet directory regardless of the wallets loaded.

One issue I ran into was dealing with encrypted wallets. Ideally, we would detect whether a wallet is encr
...

📝 achow101 opened a pull request: "wallet: Fix listwalletdir listing of migrated default wallets and generated backup files"
(https://github.com/bitcoin/bitcoin/pull/30265)
When the default wallet is migrated, we do not rename the wallet so we end up having a descriptor wallet with the empty string as its name and the wallet.dat file in the root of the walletdir. This is supposed to be an unsupported configuration and there is no other way to achieve this (other than file copying), but the wallet loading code does not disallow loading such wallets. However `listwalletdir` does not currently list the default wallet if it is sqlite. This is confusing to users, so cha
...

💬 mzumsande commented on pull request "test: Assumeutxo: import snapshot in a node with a divergent chain":
(https://github.com/bitcoin/bitcoin/pull/29996#discussion_r1633915935)
I can confirm this behavior and also think that this has uncovered a bug in `net_processing`.

I think that the root cause is in [TryDownloadingHistoricalBlocks](https://github.com/bitcoin/bitcoin/blob/b1ba1b178f501daa1afdd91f9efec34e5ec1e294/src/net_processing.cpp#L1511-L1538) (which is responsible for downloading the background chain):
This function calls `FindNextBlocks()` with `pindexWalk` set to the current tip of the background chainstate (`from_tip`), and `target_block` set to the sna
...