💬 mzumsande commented on issue "RfC: increase minimum prune target?":
(https://github.com/bitcoin/bitcoin/issues/30037#issuecomment-2096202995)
> 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 earli
...

💬 mzumsande commented on issue "Manually Banning Peers Results in Crash":
(https://github.com/bitcoin/bitcoin/issues/29916#issuecomment-2096263893)
> Could this be a race between "clicking on the peer to ban it in the GUI" and "the peer disconnecting"?

I thought the same last week and tried to reproduce it with node by adding a sleep after a node disconnected, but didn't manage to reproduce the crash (I might have put the sleep in the wrong place though...).

💬 pinheadmz commented on issue "release: ship codesigned MacOS arm64 binaries":
(https://github.com/bitcoin/bitcoin/issues/29749#issuecomment-2096273770)
I think this is a decent idea. I am a [macOS codesigner](https://github.com/bitcoin-core/bitcoin-detached-sigs/pull/34) and was able to sign a guix-built `bin/bitcoind` release as well as create a detached signature like we do for the gui. However I did so with the Apple-blessed tools (OS keychain and `codesign` command). I was also unable to re-attach that signature to the binary which would be required for guix attestations. So the work involved here would mostly be patching https://github.com
...

💬 pinheadmz commented on pull request "rpc, wallet: fix incorrect segwit redeem script size limit":
(https://github.com/bitcoin/bitcoin/pull/28307#issuecomment-2096308420)
> The confusing point about this command is that it returns the post-processed tx hex regardless of what it was able to do internally or if the script passes the verification step. This is because, in a multisig scenario, the process could have appended only one of the signatures, which would obviously fail during the script verification. Thus, the process needs to update the transaction and notify the user that there are still missing signatures so it can relay it to the next party.

Ah thank
...

💬 mannyg253 commented on pull request "Compare `COutPoints` lexicographically":
(https://github.com/bitcoin/bitcoin/pull/30046#issuecomment-2096315155)
Bitcoin

💬 instagibbs commented on pull request "rpc: improve submitpackage documentation and other improvements":
(https://github.com/bitcoin/bitcoin/pull/29292#discussion_r1591182646)
found it weird
```Suggestion
assert_raises_rpc_error(-8, f"Array must contain between 1 and {MAX_PACKAGE_COUNT} transactions.", node.submitpackage, [])
```

💬 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.