💬 kevkevinpal commented on pull request "test: add coverage to rpc_scantxoutset.py":
(https://github.com/bitcoin/bitcoin/pull/27422#issuecomment-1505538644)
> Yes can be something like `assert_raises_rpc_error(-8, "Invalid action 'word'", self.nodes[0].scantxoutset, "word")`

opened PR for this https://github.com/bitcoin/bitcoin/pull/27453
let me know if you have any comments

💬 instagibbs commented on pull request "mempool: disallow txns under min relay fee, even in packages":
(https://github.com/bitcoin/bitcoin/pull/26933#issuecomment-1505540220)
I think either way is "meaningful progress", but I'm not averse to coupling the commits if we're in agreement on this PR's commits.

💬 pinheadmz commented on pull request "Fix logging RPC and -debugexclude with 0/none values, add test coverage, improve docs":
(https://github.com/bitcoin/bitcoin/pull/27231#discussion_r1164350707)
Well I meant something like a switch statement with `"-debug"` and `"-debugexclude"` literal strings as cases. I think that would make it more readable and if anything is ever added to `DEBUG_LOG_OPTS[]` it won't necessarily break. But that might just be too long or out of style

💬 furszy commented on pull request "wallet: finish addressbook encapsulation":
(https://github.com/bitcoin/bitcoin/pull/26836#issuecomment-1505555162)
Rebased, conflicts solved.
Plus, per https://github.com/bitcoin/bitcoin/pull/26761#discussion_r1134636537 request, added coverage for wallet address book migration process.

💬 pinheadmz commented on pull request "Fix logging RPC and -debugexclude with 0/none values, add test coverage, improve docs":
(https://github.com/bitcoin/bitcoin/pull/27231#discussion_r1164360080)
Ah I see, these cases are essentially handled by `IsNoneCategory()` 👍

💬 fanquake commented on pull request "ci: use LLVM/Clang 15 and Valgrind 3.19 in Valgrind jobs":
(https://github.com/bitcoin/bitcoin/pull/27444#discussion_r1164364129)
I'll leave this for now, and follow up in the build-from-source PR

💬 fanquake commented on pull request "ci: use LLVM/Clang 15 and Valgrind 3.19 in Valgrind jobs":
(https://github.com/bitcoin/bitcoin/pull/27444#issuecomment-1505564980)
> What is the error message on aarch64? Does it happen with gcc and clang?

I'll open a separate issue for this.

I've opened some PRs in other repos in readiness for this change:
* https://github.com/bitcoin-core/qa-assets/pull/115
* https://github.com/MarcoFalke/b-c-nightly/pull/7

So going to go-ahead and merge this.

💬 kristapsk commented on pull request "RPC: Add universal options argument to listtransactions":
(https://github.com/bitcoin/bitcoin/pull/22807#issuecomment-1505567777)
Rebased

💬 MarcoFalke commented on pull request "ci: use LLVM/Clang 15 and Valgrind 3.19 in Valgrind jobs":
(https://github.com/bitcoin/bitcoin/pull/27444#discussion_r1164373407)
Any reason to use `clang-15` over `clang`? Absent a reason, this makes it harder to bump or quickly modify the image name tag

🤔 1440000bytes requested changes to a pull request: "Remove BIP35 mempool p2p message"
(https://github.com/bitcoin/bitcoin/pull/27426#pullrequestreview-1381688305)
NACK

> > It is still possible to guess mempool transactions using `getdata` by maintaining mempool with no restrictions.
>
> This doesn't work for new transactions, a node answers a `getdata` request for a tx it didn't announce to a peer via an `inv` if that transaction is older than `UNCONDITIONAL_RELAY_DELAY` (2 minutes), see [here](https://github.com/bitcoin/bitcoin/blob/d544d03ba6c7ed3c5879c8bc1108f45d0aac2798/src/net_processing.cpp#L2261-L2263). This is meant to protect against transa
...

💬 fanquake commented on pull request "ci: use LLVM/Clang 15 and Valgrind 3.19 in Valgrind jobs":
(https://github.com/bitcoin/bitcoin/pull/27444#discussion_r1164379070)
I would rather be using newer tools/compilers when available, generally less bugs/problems, but also too catch potential issues introduced by newer versions of the tools, i.e https://github.com/bitcoin-core/secp256k1/pull/1257, sooner, rather than later. However if this is going to be too annoying for hotswapping images, I can drop it.

💬 ajtowns commented on pull request "Allow configuring target block time for a signet":
(https://github.com/bitcoin/bitcoin/pull/27446#issuecomment-1505584771)
> > Configurable consensus parameters seems like a great way to make the network inconsistent in general (in particular, if you get the parameter wrong, you'll end up marking valid blocks invalid, with various consequences).
> I don't know if that is a really fair criticism for signet, `-signetchallenge` is a consensus param as well.

Two nodes with different signetchallenges will immediately realise they don't want to talk to each other -- at the p2p (and wallet, iirc) level their network Me
...

💬 MarcoFalke commented on pull request "ci: use LLVM/Clang 15 and Valgrind 3.19 in Valgrind jobs":
(https://github.com/bitcoin/bitcoin/pull/27444#discussion_r1164382055)
> using newer tools/compilers

No strong opinion, but using `clang` makes that easier, because you can just bump the tag to `ubuntu:devel` and get clang 16 that way.

💬 instagibbs commented on pull request "Remove BIP35 mempool p2p message":
(https://github.com/bitcoin/bitcoin/pull/27426#issuecomment-1505589569)
> Adding a loadmempool RPC seems to be exactly what we want here?

concept ACK on this, if that's the only use-case. We should probably offer a similar service over RPC first, in addition to information gathering that it's not actually being used in meaningful amounts in production. I've never come across usage myself in my production work.

I'm not sure where "20 lines of code" came from as it looks like at least a few multiples of that, but in general, removing a service no one uses to mak
...

💬 fanquake commented on pull request "ci: use LLVM/Clang 15 and Valgrind 3.19 in Valgrind jobs":
(https://github.com/bitcoin/bitcoin/pull/27444#discussion_r1164398634)
Ok. I'll leave this for now, so we can hotswap easier

💬 achow101 commented on pull request "Add "warnings", deprecate "warning" in {create,load,unload,restore}wallet":
(https://github.com/bitcoin/bitcoin/pull/27279#issuecomment-1505619302)
ACK 7ccdd741fe1544c13b2a9b7baa5c5727e84d6e55

💬 instagibbs commented on pull request "mempool: disallow txns under min relay fee, even in packages":
(https://github.com/bitcoin/bitcoin/pull/26933#discussion_r1164413849)
I'm confused, why is the subtraction of incremental relay total fee necessary?

💬 brunoerg commented on pull request "test: cover addrv2 anchors by adding TorV3 to CAddress in messages.py":
(https://github.com/bitcoin/bitcoin/pull/27452#discussion_r1164415030)
I think you could use `assert_equal` here and in the other similar cases

```suggestion
assert_equal(len(peer_info), 1)
```

💬 glozow commented on pull request "mempool: disallow txns under min relay fee, even in packages":
(https://github.com/bitcoin/bitcoin/pull/26933#discussion_r1164415485)
New mempool min feerate is the feerate of removed + incremental:
https://github.com/bitcoin/bitcoin/blob/7f4ab67e7be615b2425a85fb65872fc2327f58c3/src/txmempool.cpp#L1063-L1068

🚀 achow101 merged a pull request: "Add "warnings", deprecate "warning" in {create,load,unload,restore}wallet"
(https://github.com/bitcoin/bitcoin/pull/27279)