👍 TheCharlatan approved a pull request: "depends: harden libevent"
(https://github.com/bitcoin/bitcoin/pull/27118)
ACK 778e34e8625cc83d0e5a93493c71f01712bef81d

💬 glozow commented on pull request "src/node/miner cleanups, follow-ups for #26695":
(https://github.com/bitcoin/bitcoin/pull/26883#issuecomment-1436796938)
ACK 6a5e88e5cf

🚀 glozow merged a pull request: "src/node/miner cleanups, follow-ups for #26695"
(https://github.com/bitcoin/bitcoin/pull/26883)

💬 glozow commented on pull request "rpc: Add a parameter to sendrawtransaction which sets a maximum output for unspendable transactions.":
(https://github.com/bitcoin/bitcoin/pull/25943#discussion_r1111854092)
nit: (un)spendable should refer to outputs, not transactions. Same issue with the PR description.
```suggestion
- `sendrawtransaction` has a new, optional argument, `maxburnamount` with a default value of `0`. Any transaction containing an unspendable output with value greater than `maxburnamount` will not be submitted. At present, the outputs deemed unspendable are those with scripts that begin with an `OP_RETURN` code (known as 'datacarriers'), scripts that exceed the maximum script size, an
...

💬 glozow commented on pull request "rpc: Add a parameter to sendrawtransaction which sets a maximum output for unspendable transactions.":
(https://github.com/bitcoin/bitcoin/pull/25943#discussion_r1111867440)
This isn't true? If set to 0, transactions with provably unspendable outputs are fine as long as the output values are 0. Perhaps remove this line, as we wouldn't want a user to mistakenly think they're filtering out all datacarrier transactions by setting this (also, the same thing happens if this option is left blank since 0 is default).

💬 glozow commented on pull request "rpc: Add a parameter to sendrawtransaction which sets a maximum output for unspendable transactions.":
(https://github.com/bitcoin/bitcoin/pull/25943#discussion_r1111867497)
Perhaps add "If burning funds through unspendable outputs is desired, increase this value." If a downstream app is (hypothetically) regularly burning money and upgrades their bitcoind, it should be easy to understand what to do to make their error go away.

👍 TheCharlatan approved a pull request: "Convert ArgsManager::GetDataDir to a read-only function"
(https://github.com/bitcoin/bitcoin/pull/27073)
Code review ACK 0af17e161d751b15f90615800411f36e11b3fcde

💬 brunoerg commented on pull request "p2p: cleanup `LookupIntern`, `Lookup` and `LookupHost`":
(https://github.com/bitcoin/bitcoin/pull/26261#issuecomment-1436994880)
Force-pushed rebasing.

💬 vasild commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1111936979)
Done. This also reduced the size of the diff (the last commit). Thanks!

💬 vasild commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#issuecomment-1436999481)
`7c591c868d...a81fe4ff9b`: rebase and put the fuzz tests in `fuzz/process_message.cpp` instead of in a new file, as [suggested](https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1068245906).

💬 willcl-ark commented on issue "signrawtransactionwithkey command shouldn't output the "Witness program was passed an empty witness" error for a TapRoot transaction":
(https://github.com/bitcoin/bitcoin/issues/27017#issuecomment-1437012050)
The output of `help signrawtransactionwithkey` states:

```bash
3. prevtxs (json array, optional) The previous dependent transaction outputs
[
{ (json object)
"txid": "hex", (string, required) The transaction id
"vout": n, (numeric, required) The output number
"scriptPubKey": "hex", (string, required) script key
"redeemScript": "hex", (string) (required
...

💬 Hyunhum commented on pull request "script: add description for the functionality of each opcode":
(https://github.com/bitcoin/bitcoin/pull/27109#issuecomment-1437016840)
I followed the way [miniscript.h](https://github.com/bitcoin/bitcoin/blob/master/src/script/miniscript.h) comment. There are so many single lines as well.
However, I think it's also not bad to add a simple overall description and wiki link above all opcodes.

📝 brunoerg opened a pull request: "test: fix intermittent issue in `p2p_disconnect_ban`"
(https://github.com/bitcoin/bitcoin/pull/27128)
Fixes #26808

When `node0` calls `disconnectnode` to disconnect `node1`, we should check in `node1` if it worked, because for `node0` the informations in `getpeerinfo` may be updated before really completing the disconnection.

💬 furszy commented on pull request "wallet, gui: bugfix, getAvailableBalance skips selected coins":
(https://github.com/bitcoin/bitcoin/pull/26699#issuecomment-1437077463)
rebased, conflicts solved.

💬 vasild commented on pull request "p2p: return `CSubNet` in `LookupSubNet`":
(https://github.com/bitcoin/bitcoin/pull/26078#discussion_r1112010947)
The new code could call `unban()` with an invalid subnet whereas the previous one would not. `unban()` would call `m_banned.erase()` which would use `operator==(const CSubNet& a, const CSubNet& b)` which could match another invalid entry, I guess. Maybe that is ok, but to keep this change "non-functional", the new code should be:

```cpp
if (possibleSubnet.IsValid() && m_node.unban(possibleSubnet)) {
```

💬 Sjors commented on pull request "docs: add ramdisk guide for running tests on OSX":
(https://github.com/bitcoin/bitcoin/pull/27124#issuecomment-1437111734)
utACK 62ac3b4

That looks pretty much like what I use on macOS.

💬 vasild commented on pull request "Handle CJDNS from LookupSubNet()":
(https://github.com/bitcoin/bitcoin/pull/27071#issuecomment-1437113256)
> `CConnman::DisconnectNode` also handles addresses as subnets. Is this a problem?

Well, it works, but since it does something nonsensical (e.g. subnetting tor) it could be a source of confusion or ultimately, bugs. IMO it shouldn't be doing that. I think the existence and implementation of `CConnman::DisconnectNode(const CSubNet& subnet)` is ok - it will/should disconnect any IPv[46] address that matches the given IPv[46] subnet. But this one:

```cpp
bool CConnman::DisconnectNode(const C
...

💬 willcl-ark commented on pull request "docs: add ramdisk guide for running tests on OSX":
(https://github.com/bitcoin/bitcoin/pull/27124#discussion_r1112027162)
I think this could "feel safer" for people to run if it were split into two commands (even though it's using a subshell)?

💬 Sjors commented on pull request "docs: add ramdisk guide for running tests on OSX":
(https://github.com/bitcoin/bitcoin/pull/27124#discussion_r1112032998)
You need to do this again at every reboot, so it's nice to have a single incantation in your bash history.

💬 willcl-ark commented on pull request "docs: add ramdisk guide for running tests on OSX":
(https://github.com/bitcoin/bitcoin/pull/27124#discussion_r1112033932)
OK, that makes sense too :)