💬 davidgumberg commented on pull request "p2p: gives seednode priority over dnsseed if both are provided":
(https://github.com/bitcoin/bitcoin/pull/28016#issuecomment-2081559487)
reACK https://github.com/bitcoin/bitcoin/commit/82f41d76f1c6ad38290917dad5499ffbe6b3974d
Re-reviewed changes without testing

💬 davidgumberg commented on pull request "Lint: support running individual rust linters and improve subtree exclusion":
(https://github.com/bitcoin/bitcoin/pull/29965#issuecomment-2081561015)
Windows CI appears to have timed out, Rebased to get a fresh CI run.

💬 mjdietzx commented on pull request "tests: add functional test for miniscript decaying multisig":
(https://github.com/bitcoin/bitcoin/pull/29156#discussion_r1582257401)
I don't think it was ever necessary for me to demonstrate this alternative signing flow as part of this test. So I simplified and removed this code. Now all sends are tested in the single loop

💬 mjdietzx commented on pull request "tests: add functional test for miniscript decaying multisig":
(https://github.com/bitcoin/bitcoin/pull/29156#issuecomment-2081582395)
I've addressed all your comments and rebased/squashed @achow101

> Using multiple wallets is preferred over using multiple nodes.

This point led me to simplify and remove some unnecessary code/checks. When I had first implemented [test/functional/wallet_multisig_descriptor_psbt.py](https://github.com/bitcoin/bitcoin/blob/master/test/functional/wallet_multisig_descriptor_psbt.py) there was feedback/interest in different nodes/participants coordinating together. That led to some additional ch
...

💬 mjdietzx commented on pull request "tests: add functional test for miniscript decaying multisig":
(https://github.com/bitcoin/bitcoin/pull/29156#discussion_r1582265867)
After addressing comments from your review, I made some simplifications and the two tests are now different enough I think this is no longer the case. The tests are still similar in their high-level structure, but otherwise not much code is duplicated (and definitely no complex/deep logic)

💬 laanwj commented on pull request "depends: Fix build of Qt for 32-bit platforms with recent glibc":
(https://github.com/bitcoin/bitcoin/pull/29985#discussion_r1582279290)
Thanks, will update

💬 fjahr commented on pull request "prune, rpc: Check undo data when finding pruneheight":
(https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1582311337)
done

💬 fjahr commented on pull request "prune, rpc: Check undo data when finding pruneheight":
(https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1582311605)
done, I liked that `BLOCK_HAVE_DATA | BLOCK_HAVE_UNDO` is more expressive because we don't use `BLOCK_HAVE_MASK` much I feel like people would need to look it up more, but I have just added a comment instead now.

💬 fjahr commented on pull request "prune, rpc: Check undo data when finding pruneheight":
(https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1582311745)
done

💬 fjahr commented on pull request "prune, rpc: Check undo data when finding pruneheight":
(https://github.com/bitcoin/bitcoin/pull/29668#issuecomment-2081618206)
@stickies-v Thank you, I mostly took your suggestions with some minor tweaks and made you co-author of the refactoring commit. I was a bit torn about letting the caller handle the Genesis block issue but since this was actually the previous behavior I guess it's better to not change it if you like it the way it is. I have adapted `GetPruneHeight` to return `std::optional` which I found a good fit and avoids the `-1` return value. And I think the change in `init.cpp` isn't needed here (yet). But
...

💬 fjahr commented on pull request "RPC: access RPC arguments by name":
(https://github.com/bitcoin/bitcoin/pull/29277#issuecomment-2081631545)
Code review ACK 30a6c999351041d6a1e8712a9659be1296a1b46a

I still prefer my suggested approach but it's still an improvement and I don't want to block this.

📝 sdaftuar opened a pull request: "fuzz: don't allow adding duplicate transactions to the mempool"
(https://github.com/bitcoin/bitcoin/pull/29990)
Filter duplicate transaction ids from being added to the mempool in the `partially_downloaded_block` fuzz target.

I think a prerequisite for calling `CTxMemPool::addUnchecked` should be that the underlying txid doesn't already exist in the mempool (otherwise `addUnchecked` would need a way to return failure, which we don't currently have).

🤔 tdb3 reviewed a pull request: "RPC: Return `permitbaremultisig` and `maxdatacarriersize` in `getmempoolinfo`"
(https://github.com/bitcoin/bitcoin/pull/29954#pullrequestreview-2027366385)
Concept ACK.
Thank you. Appears to work well. Seems helpful to me for clients to have this information provided through RPC. I took a quick look at other RPC calls (in https://bitcoincore.org/en/doc/27.0.0/), but didn't see any that had `permitbaremultisig` and `maxdatacarriersize`.

Recently, Issue #29912 was opened, which discusses implementing a formal specification for the RPC API. RPC API changes have the potential to adjust the meaning, fields/types, and/or values used in or returne
...

💬 tdb3 commented on pull request "RPC: Return `permitbaremultisig` and `maxdatacarriersize` in `getmempoolinfo`":
(https://github.com/bitcoin/bitcoin/pull/29954#discussion_r1582400466)
I second this. It would add value to have tests that cover:
1) permitbaremultisig disabled
2) permitbaremultisig enabled

This might be accomplished through the use of a second node (`self.num_nodes`) or perhaps through stopping the node, modifying configuration with `replace_in_config()`, and restarting it. Typically num_nodes is to be minimized and start/stops avoided when possible, but I do not immediately see how different `permitbaremultisig` states could be tested without doing one o
...

👍 cbergqvist approved a pull request: "test: adds outbound eviction functional tests, updates comment in ConsiderEviction"
(https://github.com/bitcoin/bitcoin/pull/29122#pullrequestreview-2027367015)
ACK d53d84834747c37f4060a9ef379e0a6b50155246

Functional including `--extended` tests passed (skipped unrelated `feature_dbcrash`).

💬 tdb3 commented on issue "RFC: Formal description of the RPC API":
(https://github.com/bitcoin/bitcoin/issues/29912#issuecomment-2081678433)
Concept ACK. IMHO, a formal specification (especially machine ingestible) would add value for downstream clients.

There is some great info/guidance in
https://github.com/bitcoin/bitcoin/blob/master/doc/JSON-RPC-interface.md#versioning
https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#rpc-interface-guidelines

Maybe it's just a matter of adding content to the RPC interface guidelines section of the developer notes, but I think there is value in defining the approach t
...

👍 cbergqvist approved a pull request: "p2p: gives seednode priority over dnsseed if both are provided"
(https://github.com/bitcoin/bitcoin/pull/28016#pullrequestreview-2027389319)
ACK 82f41d76f1c6ad38290917dad5499ffbe6b3974d

After checking out ran: `git range-diff 2842e51~2..2842e51 HEAD~2..HEAD`
Functional including `--extended` tests passed (skipped unrelated `feature_dbcrash`).

💬 RandyMcMillan commented on pull request "tests: add functional test for miniscript decaying multisig":
(https://github.com/bitcoin/bitcoin/pull/29156#issuecomment-2081684905)
Concept ACK 7ddfc6d

📝 fanquake locked a pull request: "Update bitcoin_config.h.in"
(https://github.com/bitcoin/bitcoin/pull/29988)
<!--
*** Please remove the following help text before submitting: *** Pull requests without a rationale and clear improvement may be closed
immediately. GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that improve cove
...

🚀 fanquake merged a pull request: "guix: remove bzip2 from deps"
(https://github.com/bitcoin/bitcoin/pull/29895)