💬 ajtowns commented on pull request "test: create assert_not_equal util":
(https://github.com/bitcoin/bitcoin/pull/29500#discussion_r1606772606)
This error message doesn't make sense when `*args` is non-empty: if you say `assert_not_equal(a, 1, 2)` and `a` is 1 or 2, it will report `1 == 1 == 2` or `2 == 1 == 2`.

💬 marcofleon commented on pull request "fuzz: add more coverage for `ScriptPubKeyMan`":
(https://github.com/bitcoin/bitcoin/pull/30134#issuecomment-2120491389)
Got it, thanks @brunoerg. I'll redo and get back to you then. I also realized I was looking at the wrong line in the coverage report. I should probably be looking at `wallet/scriptpubkeyman.cpp` not `wallet/test/fuzz/scriptpubkeyman.cpp`.

💬 maflcko commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1606802910)
Looks like this is unused?

👍 willcl-ark approved a pull request: "net: make the list of known message types a compile time constant"
(https://github.com/bitcoin/bitcoin/pull/29421#pullrequestreview-2066265255)
ACK b3efb486732f3caf8b8a8e9d744e6d20ae4255ef

Based on range diff from previous ACK, all looks good!
`range-diff 2e312ea...b3efb48`

💬 stickies-v commented on pull request "indexes: Don't wipe indexes again when continuing a prior reindex":
(https://github.com/bitcoin/bitcoin/pull/30132#discussion_r1606818901)
As discussed offline, I think this change means we're ignoring the GUI-based reindex from https://github.com/bitcoin/bitcoin/pull/30132/files#diff-b1e19192258d83199d8adaa5ac31f067af98f63554bfdd679bd8e8073815e69dR1609, both for these optional indices but also for the block index

💬 maflcko commented on pull request "wallet: Allow user to navigate options while encrypting at creation":
(https://github.com/bitcoin-core/gui/pull/722#issuecomment-2120525642)
I don't understand this change. It looks like a "Back" button is added to the warning pop-up, but it will only take you back to enter a different password, not to disable the password selection in the create wallet dialog. Wouldn't it be better to add the "Back" button to the password dialog?

Also, wouldn't it be better if the text from the two warning pop-ups was added to the top of the password dialog? Otherwise, it seems odd to first ask for a password from the user, then print two warning
...

💬 maflcko commented on pull request "wallet: Allow user to navigate options while encrypting at creation":
(https://github.com/bitcoin-core/gui/pull/722#issuecomment-2120529217)
Also, this doesn't need release notes, does it?

💬 TheCharlatan commented on pull request "indexes: Don't wipe indexes again when continuing a prior reindex":
(https://github.com/bitcoin/bitcoin/pull/30132#issuecomment-2120534220)
Updated 991f50acad6809c83c0c44cf3c55fb9d2c0107e3 -> 9de8b263dabd6dd2f86f1f0253c6ee3fac7a4407 ([preserveIndexOnRestart_0](https://github.com/TheCharlatan/bitcoin/tree/preserveIndexOnRestart_0) -> [preserveIndexOnRestart_1](https://github.com/TheCharlatan/bitcoin/tree/preserveIndexOnRestart_1), [compare](https://github.com/TheCharlatan/bitcoin/compare/preserveIndexOnRestart_0..preserveIndexOnRestart_1))

* Addressed @stickies-v's [comment](https://github.com/bitcoin/bitcoin/pull/30132#discussion
...

💬 instagibbs commented on pull request "refactor prep for package rbf":
(https://github.com/bitcoin/bitcoin/pull/30072#discussion_r1606835018)
done

💬 instagibbs commented on pull request "refactor prep for package rbf":
(https://github.com/bitcoin/bitcoin/pull/30072#discussion_r1606836006)
added an explicit `m_allow_sibling_eviction` ATMPArg and had it being true imply `m_allow_replacements`.

👍 willcl-ark approved a pull request: "chainparams: Add achow101 DNS seeder"
(https://github.com/bitcoin/bitcoin/pull/30007#pullrequestreview-2066317042)
ACK ee218aa9a9eaac53030c31b099b4afe354197ba7

I checked that these seeds return diverse and (mainly) active node addresses (although not in a scripted way, but by `addnode`-ing a random selection of each).

I also checked DNSSEC setup using Verisign's DNSSEC [debugger](https://dnssec-debugger.verisignlabs.com) tool for each of them, which they all passed.

💬 instagibbs commented on pull request "refactor prep for package rbf":
(https://github.com/bitcoin/bitcoin/pull/30072#discussion_r1606840132)
@sr-gi I reverted the logic change here to reduce review surface, just shortening the comment here instead.

💬 maflcko commented on pull request "Lint: support running individual rust linters and improve subtree exclusion":
(https://github.com/bitcoin/bitcoin/pull/29965#issuecomment-2120556703)
Looks like the conflicting https://github.com/bitcoin/bitcoin/pull/30034 is close(r) to merge, if you want to review it

💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#issuecomment-2120592803)
rebased on latest #30072

💬 maflcko commented on pull request "rpc: Avoid getchaintxstats invalid results":
(https://github.com/bitcoin/bitcoin/pull/29720#issuecomment-2120602326)
@fjahr Thanks! I've pushed your test commit with small fixups.

💬 edilmedeiros commented on pull request "Update manpage descriptions":
(https://github.com/bitcoin/bitcoin/pull/29686#discussion_r1606890727)
I just realized that `bitcoin-util` does not have a `help` command like `bitcoin-cli`. Looks silly now as it has only one command, but if the tool is to increase, this seems to be a fairly easy improvement someone else can work on. In case there's another PR for that, let's try to make this more close to the `bitcoin-cli` message mentioning the help command only.

💬 maflcko commented on pull request "rpc: avoid copying into UniValue":
(https://github.com/bitcoin/bitcoin/pull/30115#issuecomment-2120649012)
Maybe rebase and re-bench after 75118a608fc22a57567743000d636bc1f969f748, which replaced some copies with moves as well?

💬 theuni commented on pull request "rpc: avoid copying into UniValue":
(https://github.com/bitcoin/bitcoin/pull/30115#issuecomment-2120681426)
> Maybe rebase and re-bench after [75118a6](https://github.com/bitcoin/bitcoin/commit/75118a608fc22a57567743000d636bc1f969f748), which replaced some copies with moves as well?

Ah, great, thanks for pointing me to that. I had pretty much this same commit locally to address the third point in this PR's description:
> - Refactoring functions to accept UniValues by value rather than by const reference

Though I think there are still other functions that need the same treatment.

💬 instagibbs commented on pull request "refactor prep for package rbf":
(https://github.com/bitcoin/bitcoin/pull/30072#issuecomment-2120685605)
win64 test failure is minisketch https://github.com/bitcoin/bitcoin/pull/30072/checks?check_run_id=25181774728

💬 ryanofsky commented on pull request "indexes: Don't wipe indexes again when continuing a prior reindex":
(https://github.com/bitcoin/bitcoin/pull/30132#issuecomment-2120741121)
> fixing a bug introduced in b47bd959207e82555f07e028cc2246943d32d4c3: "kernel: De-globalize fReindex".

Is this true? That commit, which was part of #29817, should not have changed any previous behavior