💬 instagibbs commented on pull request "rpc: improve submitpackage documentation and other improvements":
(https://github.com/bitcoin/bitcoin/pull/29292#discussion_r1591189552)
if we're lining up ala `testmempoolaccept` shouldn't we be adding single quotes around the whole array as well?

👍 pinheadmz approved a pull request: "rpc, wallet: fix incorrect segwit redeem script size limit"
(https://github.com/bitcoin/bitcoin/pull/28307#pullrequestreview-2041087253)
ACK 2451a217dd2c21b6d2f2b2699ceddd0bf9073019

Minimal diff since last ACK, just rebase on master and replace a"520" with a "MAX_SCRIPT_ELEMENT_SIZE"

<details><summary>Show Signature</summary>

```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

ACK 2451a217dd2c21b6d2f2b2699ceddd0bf9073019
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmY4+ZIACgkQ5+KYS2KJ
yTqhnhAApaBq0dhQVXvLaTSBjV8Q8CXJTXapgme6uII649DlRjCQujM4ufmnPfdR
BuQWzAC5FbgA1sCkRk3+D/CyO0H9k5PP
...

💬 pinheadmz commented on pull request "include verbose "debug-message" field in testmempoolaccept response":
(https://github.com/bitcoin/bitcoin/pull/28121#issuecomment-2096338515)
> Other reasons like e.g. `scriptpubkey`, `tx-size`, `dust`, `missing-inputs`, `multi-op-return` have an empty debug-message.

debug-message was empty like `""`? I'd expect the key to not be present at all:

```cpp
if (!state.GetDebugMessage().empty()) result_inner.pushKV("debug-message", state.GetDebugMessage());
```

✅ instagibbs closed a pull request: "p2p: Allow 1P1C to fetch parent from compact block extra_txn"
(https://github.com/bitcoin/bitcoin/pull/30032)

💬 instagibbs commented on pull request "p2p: Allow 1P1C to fetch parent from compact block extra_txn":
(https://github.com/bitcoin/bitcoin/pull/30032#issuecomment-2096341611)
Don't think I find these results compelling vs the complexity in how we would need to handle the packages

👍 ryanofsky approved a pull request: "doc: fix broken relative md links"
(https://github.com/bitcoin/bitcoin/pull/30025#pullrequestreview-2041170570)
Code review ACK 4b9f49da2b120e81516ddc3dc577d7a2e58e02d3. Thanks for the updates!

💬 ryanofsky commented on pull request "doc: fix broken relative md links":
(https://github.com/bitcoin/bitcoin/pull/30025#discussion_r1591242759)
In commit "doc: fix broken relative md links" (4b9f49da2b120e81516ddc3dc577d7a2e58e02d3)

This is also updating the flake8 project url, not just changing links to be absolute, so could mention this in the commit message

💬 josibake commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1591254268)
Closed #30046 :smile:

🚀 achow101 merged a pull request: "rpc: return warnings as an array instead of just a single one"
(https://github.com/bitcoin/bitcoin/pull/29845)

💬 0xB10C commented on pull request "include verbose "debug-message" field in testmempoolaccept response":
(https://github.com/bitcoin/bitcoin/pull/28121#issuecomment-2096448043)
> debug-message was empty like `""`? I'd expect the key to not be present at all:

You're right. I should have been clearer in my comment: the field wasn't present. My tooling [handles](https://github.com/0xB10C/find-non-standard-tx/pull/2/files#diff-42cb6807ad74b3e201c5a7ca98b911c5fa08380e942be6e4ac5807f8377f87fcR144) the missing field by setting the value to "".

💬 theuni commented on pull request "net: Replace libnatpmp with built-in PCP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2096485412)
Whoa :)

💬 tdb3 commented on issue "RfC: increase minimum prune target?":
(https://github.com/bitcoin/bitcoin/issues/30037#issuecomment-2096487922)
> > since nodes with limited space would believe bitcoind was being limited/constrained to a particular allocated amount of space but would be exceeding the allocated amount
>
> This is already the case today, because the 288 blocks rule takes precedence over the user-provided number. With average block sizes of 1.6MiB currently, my synced `prune=550` node usually takes up between 700MiB and 900MiB of space for `*.blk` and `*.rev` files.. The target is only respected when possible (during the
...

🤔 Sjors reviewed a pull request: "net: Replace libnatpmp with built-in PCP implementation"
(https://github.com/bitcoin/bitcoin/pull/30043#pullrequestreview-2040718091)
RFC6887 appendix A describes how a router that supports NAP-PMP but not PCP, will return `UNSUPP_VERSION`. A log message could encourage users to try `-upnp` instead (if not already enabled). Or upgrade their ancient router firmware :-)

Got distracted during review, will continue later. I'll look into how we can preserve a previously selected NatPMP checkbox in the GUI.

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1590975165)
a2d67c320f8a28da98e6c8352bd67648a9b831a8: I think you need to keep this (and above) until `-natpmp` is removed.

💬 murchandamus commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2096505344)
> > Prevent use of the 20-minute difficulty exception on the last block in a difficulty period
>
> @murchandamus do you explicitly prefer this over my proposed fix? I find it more elegant and intuitive if we allow the use on all blocks and prevent the adjustment from being affected by it.

> > Alternatively, I would be satisfied if using the 20-minute difficulty exception would be changed such that nBits could remain at the proper value, and were available for any block in the difficulty pe
...

💬 sr-gi commented on pull request "test/BIP324: disconnection scenarios during v2 handshake":
(https://github.com/bitcoin/bitcoin/pull/29431#issuecomment-2096517368)
I find the approach fairly hard to follow here. Having all the logic in the constructor with if/elses based on the connection type instead of having different constructors/a test for each type of fail feels really error-prone and difficult to make sure if a test is doing what we expect. I think it'd be better to have multiple classes that build from `v2_p2p` and modify what's needed, even if the file gets larger.

Just an example:

In the `WRONG_GARBAGE_TERMINATOR` case, we are modifying the
...

👍 ryanofsky approved a pull request: "Logging cleanup"
(https://github.com/bitcoin/bitcoin/pull/29798#pullrequestreview-2041268723)
Code review ACK df2422c5f91e5b1b42d6b718cc2527dc413712fc. Looks good, this gets rid of some unnecessary complexity and strange behavior in logging RPC.

💬 ryanofsky commented on pull request "Logging cleanup":
(https://github.com/bitcoin/bitcoin/pull/29798#discussion_r1591311644)
> > Right now this returns NONE/true when the empty string "" is passed.
>
> Hmm, no? It returns ALL/true when "" is passed, in `master` and in this PR - there is an `if (str.empty() ...` a few lines above.

Right, I think I was confused when I wrote that comment. Makes sense to keep "" behavior unchanged, too.

👍 theuni approved a pull request: "net: Replace ifname check with IFF_LOOPBACK in Discover"
(https://github.com/bitcoin/bitcoin/pull/29984#pullrequestreview-2041299260)
utACK a68fed111be393ddbbcd7451f78bc63601253ee0. Satoshi-era brittleness :)

💬 theuni commented on pull request "depends: sqlite 3.45.3":
(https://github.com/bitcoin/bitcoin/pull/29991#issuecomment-2096548294)
Any particular reason for the bump?