🚀 fanquake merged a pull request: "doc: update OpenBSD build docs for 7.3 (external signer support available)"
(https://github.com/bitcoin/bitcoin/pull/27449)
(https://github.com/bitcoin/bitcoin/pull/27449)
💬 pinheadmz commented on pull request "test: add support for all networks in `deserialize_v2`":
(https://github.com/bitcoin/bitcoin/pull/27295#discussion_r1164252589)
Is this correct for torv3? I had to add version and checksum in https://github.com/bitcoin/bitcoin/pull/27452
(https://github.com/bitcoin/bitcoin/pull/27295#discussion_r1164252589)
Is this correct for torv3? I had to add version and checksum in https://github.com/bitcoin/bitcoin/pull/27452
💬 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.
(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...
(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?
(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
(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
...
(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
(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
(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.
(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
(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.
(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()` 👍
(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
(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.
(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
(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
(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
...
(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.
(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
...
(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
...