💬 ryanofsky commented on pull request "refactor: Extract common/args from util/system":
(https://github.com/bitcoin/bitcoin/pull/27419#issuecomment-1505447794)
I think this PR could be labeled move-only instead of refactoring, because no code is changing other than #includes.

💬 ajtowns commented on pull request "mempool: disallow txns under min relay fee, even in packages":
(https://github.com/bitcoin/bitcoin/pull/26933#issuecomment-1505483056)
I guess I don't really understand how this PR on its own helps anything -- we don't do package relay yet, so except for a regtest only rpc, these changes should be unobservable?

Would it make sense to replace this with a PR that enabled package relay without v3 or package RBF, and included this policy? I think that would be the first ~10 commits from #25038? That seems like it would be more meaningful progress...

💬 ajtowns commented on pull request "mempool / miner: regularly flush <=0-fee entries and mine everything in the mempool":
(https://github.com/bitcoin/bitcoin/pull/27018#discussion_r1164322514)
Yeah, fair point -- the claim should be "setting -blockminfee higher than zero may result in txs remaining in your mempool that will never be mined"; but as long as that's not the default behaviour, that doesn't seem particularly problematic? (TBH, I'd rather be able to pass blockminfee/blockmaxweight as getblocktemplate parameters)

Sounds like this should be closed though?

💬 ryanofsky commented on pull request "refactor: Remove CAddressBookData::destdata":
(https://github.com/bitcoin/bitcoin/pull/27224#issuecomment-1505513192)
Rebased d34323544d7283bf46bb154a90b8feca443b103e -> a5986e82dd2b8f923d60f9e31760ef62b9010105 ([`pr/nodest.19`](https://github.com/ryanofsky/bitcoin/commits/pr/nodest.19) -> [`pr/nodest.20`](https://github.com/ryanofsky/bitcoin/commits/pr/nodest.20), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/nodest.19-rebase..pr/nodest.20)) due to conflict with #27217

👍 pinheadmz approved a pull request: "Add "warnings", deprecate "warning" in {create,load,unload,restore}wallet"
(https://github.com/bitcoin/bitcoin/pull/27279#pullrequestreview-1381620202)
re-ACK 7ccdd741fe1544c13b2a9b7baa5c5727e84d6e55

confirmed trivial diff since last review, ran unit and functional tests locally, ran in regtest one more time to check UX with and without deprecated rpc flag

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

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

ACK 7ccdd741fe1544c13b2a9b7baa5c5727e84d6e55
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmQ20YEACgkQ5+KYS2KJ
yTqF/Q/8DnfN9Esae47Y1bvswkY5oX93SBB59pU
...

📝 kevkevinpal opened a pull request: "test: added coverage to rpc_cantxoutset.py"
(https://github.com/bitcoin/bitcoin/pull/27453)
Included a test that checks if an invalid first argument is entered we receive a rpc error. The rpc should fail if "start", "status" or "abort" is not the first command.

Relavant: mentioned in https://github.com/bitcoin/bitcoin/pull/27422

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