💬 1440000bytes commented on pull request "script: add description for the functionality of each opcode":
(https://github.com/bitcoin/bitcoin/pull/27109#issuecomment-1436763116)
> > It will help a lot for developers who want to do script programming.
>
> No strong opinion, but this purpose is already served by this [detailed wiki article](https://en.bitcoin.it/wiki/Script#Opcodes) presenting how each opcode works along with a bit of history. Using a "opcode, input, output, description" table it is even probably better at explaining what each opcode does than we can ever get in code comments.

Agree with this. So many single line comments look weird. If wiki has som
...

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