💬 theStack commented on issue "depends: `touch` command for creating determinstic archive timestamps fails on OpenBSD":
(https://github.com/bitcoin/bitcoin/issues/29447#issuecomment-1951391552)
ping @hebasto
(Haven't investigated deeper what `-h` exactly does, but as far as I understand, the build still succeeds and there is no big harm in the failing `touch` command other than that the timestamps are not determinstic. Considering that OpenBSD is rather a niche OS, I think it's only worth fixing if there's an easy way to do so.)
(https://github.com/bitcoin/bitcoin/issues/29447#issuecomment-1951391552)
ping @hebasto
(Haven't investigated deeper what `-h` exactly does, but as far as I understand, the build still succeeds and there is no big harm in the failing `touch` command other than that the timestamps are not determinstic. Considering that OpenBSD is rather a niche OS, I think it's only worth fixing if there's an easy way to do so.)
🤔 fjahr reviewed a pull request: "doc: explain what the wallet password does"
(https://github.com/bitcoin/bitcoin/pull/28974#pullrequestreview-1887288612)
I would integrate this into the managing wallets doc rather than creating a new file. I also agree with @maflcko that putting this information into the users' path makes sense but I think this can still be added in the docs. It may not be enough to close the issue or maybe there should be a follow-up issue giving more detail.
(https://github.com/bitcoin/bitcoin/pull/28974#pullrequestreview-1887288612)
I would integrate this into the managing wallets doc rather than creating a new file. I also agree with @maflcko that putting this information into the users' path makes sense but I think this can still be added in the docs. It may not be enough to close the issue or maybe there should be a follow-up issue giving more detail.
💬 fjahr commented on pull request "doc: explain what the wallet password does":
(https://github.com/bitcoin/bitcoin/pull/28974#discussion_r1493809517)
I think this part can be shortened a lot.
(https://github.com/bitcoin/bitcoin/pull/28974#discussion_r1493809517)
I think this part can be shortened a lot.
💬 fjahr commented on pull request "doc: explain what the wallet password does":
(https://github.com/bitcoin/bitcoin/pull/28974#discussion_r1493809487)
I don't think this is very relevant, even though it was suggested in the original issue.
(https://github.com/bitcoin/bitcoin/pull/28974#discussion_r1493809487)
I don't think this is very relevant, even though it was suggested in the original issue.
💬 fjahr commented on pull request "log: Nuke error(...)":
(https://github.com/bitcoin/bitcoin/pull/29236#issuecomment-1951396971)
utACK fa1d4f07c3
There is still a reference to `error` in the comments in `logging.h`: https://github.com/fjahr/bitcoin/commit/6457d77e39a8c07fdef798f3bf115fcbb30545c4 But removing it can be kept for a follow-up.
(https://github.com/bitcoin/bitcoin/pull/29236#issuecomment-1951396971)
utACK fa1d4f07c3
There is still a reference to `error` in the comments in `logging.h`: https://github.com/fjahr/bitcoin/commit/6457d77e39a8c07fdef798f3bf115fcbb30545c4 But removing it can be kept for a follow-up.
🤔 fjahr reviewed a pull request: "RPC: access RPC arguments by name"
(https://github.com/bitcoin/bitcoin/pull/29277#pullrequestreview-1887298400)
Concept ACK, certainly makes the RPC code more readable.
@stickies-v have you considered preparing a map once when an RPC is called rather than doing the search for the index every time an arg is accessed?
(https://github.com/bitcoin/bitcoin/pull/29277#pullrequestreview-1887298400)
Concept ACK, certainly makes the RPC code more readable.
@stickies-v have you considered preparing a map once when an RPC is called rather than doing the search for the index every time an arg is accessed?
💬 fjahr commented on pull request "RPC: access RPC arguments by name":
(https://github.com/bitcoin/bitcoin/pull/29277#discussion_r1493816010)
Indentation seems wrong?
(https://github.com/bitcoin/bitcoin/pull/29277#discussion_r1493816010)
Indentation seems wrong?
💬 sdaftuar commented on pull request "policy: enable sibling eviction for v3 transactions":
(https://github.com/bitcoin/bitcoin/pull/29306#discussion_r1493837935)
Should we also exclude the case where the sibling transaction has descendants of its own, which can happen as the result of a reorg? My concern is that our RBF rules today don't enforce incentive compatibility in such a situation, so it's not clear that we should support this case for now (post-cluster-mempool, this would be fine).
(https://github.com/bitcoin/bitcoin/pull/29306#discussion_r1493837935)
Should we also exclude the case where the sibling transaction has descendants of its own, which can happen as the result of a reorg? My concern is that our RBF rules today don't enforce incentive compatibility in such a situation, so it's not clear that we should support this case for now (post-cluster-mempool, this would be fine).
🤔 fjahr reviewed a pull request: "test: p2p: check disconnect due to lack of desirable service flags"
(https://github.com/bitcoin/bitcoin/pull/29279#pullrequestreview-1887366958)
tACK d82eafb173d6bfa98a59e86a845013cc8528b65d
(https://github.com/bitcoin/bitcoin/pull/29279#pullrequestreview-1887366958)
tACK d82eafb173d6bfa98a59e86a845013cc8528b65d
💬 fjahr commented on pull request "test: p2p: check disconnect due to lack of desirable service flags":
(https://github.com/bitcoin/bitcoin/pull/29279#discussion_r1493880488)
nit: since we wait for the disconnect anyway it seems that passing a different `p2p_idx` each time isn't necessary, so the test could be simplified a tiny bit
(https://github.com/bitcoin/bitcoin/pull/29279#discussion_r1493880488)
nit: since we wait for the disconnect anyway it seems that passing a different `p2p_idx` each time isn't necessary, so the test could be simplified a tiny bit
🤔 tdb3 reviewed a pull request: "test: test_bitcoin: allow -testdatadir=<datadir>"
(https://github.com/bitcoin/bitcoin/pull/26564#pullrequestreview-1887381174)
Nice feature. Concept ACK.
Ran a few practical tests on 7b81dea7eb6b14a939230477835159dad6b4b75b. Unless I'm overlooking something, seems like there may be an issue with some tests when `testdatadir` is provided a relative path (i.e. non-absolute path).
1. Ran all unit tests, without `-testdatadir`. All passed. Verified `/tmp/test_common_Bitcoin Core` was empty afterward.
1. Ran all unit tests, with `-testdatadir=/absolute/path/to/mytestdir/newlycreateddir/`, where `mytestdir` was cre
...
(https://github.com/bitcoin/bitcoin/pull/26564#pullrequestreview-1887381174)
Nice feature. Concept ACK.
Ran a few practical tests on 7b81dea7eb6b14a939230477835159dad6b4b75b. Unless I'm overlooking something, seems like there may be an issue with some tests when `testdatadir` is provided a relative path (i.e. non-absolute path).
1. Ran all unit tests, without `-testdatadir`. All passed. Verified `/tmp/test_common_Bitcoin Core` was empty afterward.
1. Ran all unit tests, with `-testdatadir=/absolute/path/to/mytestdir/newlycreateddir/`, where `mytestdir` was cre
...
📝 ariard opened a pull request: "Add issuer-selected opt-in txn / pckg policy checks"
(https://github.com/bitcoin/bitcoin/pull/29448)
All the values selected by the transaction issuers are implemented to respect current hard policy limits as of 27.0
Bitcoin Core version (e.g `MAX_PACKAGE_COUNT` or `MAX_STANDARD_TX_WEIGHT`). As such introducing a new distinction in the Bitcoin ecosystem among tx-relay policy checks enforced by full-nodes hosts of a said full-node implementation/version and the the policy-check opt-ed by any transaction or second-layers use-cases. That way any significant on-chain economic traffic can be still
...
(https://github.com/bitcoin/bitcoin/pull/29448)
All the values selected by the transaction issuers are implemented to respect current hard policy limits as of 27.0
Bitcoin Core version (e.g `MAX_PACKAGE_COUNT` or `MAX_STANDARD_TX_WEIGHT`). As such introducing a new distinction in the Bitcoin ecosystem among tx-relay policy checks enforced by full-nodes hosts of a said full-node implementation/version and the the policy-check opt-ed by any transaction or second-layers use-cases. That way any significant on-chain economic traffic can be still
...
💬 ariard commented on pull request "Add imbued v3 based on template-matching":
(https://github.com/bitcoin/bitcoin/pull/29427#issuecomment-1951637283)
can you precise where the imbuance mechanism should be applied? like in `AcceptSingleTransaction` or `AcceptMultipleTransaction` or even earlier at the transaction-relay level in `ProcessMessages` ?
i think it’s good thing to have a generic imbuance mechanism though i would say the earlier in the net processing stack the better.
hardcoding a template in the imbuance mechanism is a dumb idea even for today Lightning, the “must have exactly 2 330-satoshi outputs” i can just add another HTLC p2
...
(https://github.com/bitcoin/bitcoin/pull/29427#issuecomment-1951637283)
can you precise where the imbuance mechanism should be applied? like in `AcceptSingleTransaction` or `AcceptMultipleTransaction` or even earlier at the transaction-relay level in `ProcessMessages` ?
i think it’s good thing to have a generic imbuance mechanism though i would say the earlier in the net processing stack the better.
hardcoding a template in the imbuance mechanism is a dumb idea even for today Lightning, the “must have exactly 2 330-satoshi outputs” i can just add another HTLC p2
...
💬 ariard commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#issuecomment-1951640940)
do you have a simulation environment or script like you’ve done for #25717 to observe the computational complexity diff compared to today’s mempool against different chain of transaction performance payload ?
interesting what is the lowest performance linux host assumed for decentralization of the tx-relay network.
assume always 24/7 internet and on a vpcu instance, not baremetal.
(https://github.com/bitcoin/bitcoin/pull/28676#issuecomment-1951640940)
do you have a simulation environment or script like you’ve done for #25717 to observe the computational complexity diff compared to today’s mempool against different chain of transaction performance payload ?
interesting what is the lowest performance linux host assumed for decentralization of the tx-relay network.
assume always 24/7 internet and on a vpcu instance, not baremetal.
💬 ariard commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1951641928)
> are you still interested if I do a proof-of-concept bitcoin core branch or idea draft of a) moving the “package limit” on the use-case side and b) make it dynamic (in the limit of absolute core DoS robustness) ? I think it can be backup in future bip331 changes. v3 is your work and this is your PR making anti-pinning claims after-all.
did it with #29448
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1951641928)
> are you still interested if I do a proof-of-concept bitcoin core branch or idea draft of a) moving the “package limit” on the use-case side and b) make it dynamic (in the limit of absolute core DoS robustness) ? I think it can be backup in future bip331 changes. v3 is your work and this is your PR making anti-pinning claims after-all.
did it with #29448
💬 stratospher commented on pull request "test/BIP324: disconnection scenarios during v2 handshake":
(https://github.com/bitcoin/bitcoin/pull/29431#discussion_r1494116658)
`test_earlykeyresponse` sends handshake bytes etc.. in the `MainThread` and I feel it's better to isolate that behaviour in a different test.
also, if you grep for `def test_` in `test/functional`, you'll see many [examples](https://github.com/bitcoin/bitcoin/blob/3cbc8cbc71d3d6ecfaf41164ce59c24ac94bae99/test/functional/feature_asmap.py#L128) of different tests listed in `run_test()` using the pattern which is currently used.
(https://github.com/bitcoin/bitcoin/pull/29431#discussion_r1494116658)
`test_earlykeyresponse` sends handshake bytes etc.. in the `MainThread` and I feel it's better to isolate that behaviour in a different test.
also, if you grep for `def test_` in `test/functional`, you'll see many [examples](https://github.com/bitcoin/bitcoin/blob/3cbc8cbc71d3d6ecfaf41164ce59c24ac94bae99/test/functional/feature_asmap.py#L128) of different tests listed in `run_test()` using the pattern which is currently used.
💬 stratospher commented on pull request "test/BIP324: disconnection scenarios during v2 handshake":
(https://github.com/bitcoin/bitcoin/pull/29431#discussion_r1494131410)
yeah! `expected_debug_message` on [L180](https://github.com/bitcoin/bitcoin/blob/ec9005ca4be088dfa2d247bbfe964a9c98e4f29d/test/functional/p2p_v2_misbehaving.py#L180) was written with the assumption that `test_v2disconnection` is run after `test_earlykeyresponse`. so peer=0 happens in `test_earlykeyresponse` and peer=1,2,3,4,5 happens in `test_v2disconnection`.
(https://github.com/bitcoin/bitcoin/pull/29431#discussion_r1494131410)
yeah! `expected_debug_message` on [L180](https://github.com/bitcoin/bitcoin/blob/ec9005ca4be088dfa2d247bbfe964a9c98e4f29d/test/functional/p2p_v2_misbehaving.py#L180) was written with the assumption that `test_v2disconnection` is run after `test_earlykeyresponse`. so peer=0 happens in `test_earlykeyresponse` and peer=1,2,3,4,5 happens in `test_v2disconnection`.
💬 Sjors commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#discussion_r1494191408)
Because I want to drop that in #26201. Though perhaps Silent Payments itself is a reason to keep it.
(https://github.com/bitcoin/bitcoin/pull/28241#discussion_r1494191408)
Because I want to drop that in #26201. Though perhaps Silent Payments itself is a reason to keep it.
💬 paplorinc commented on pull request "doc: Bump copyright years to present (headers only)":
(https://github.com/bitcoin/bitcoin/pull/26817#issuecomment-1951977757)
I've opened a similar issue proposing to remove all of these inconsistent and hard to maintain headers and replace them with simple SPDX-License-Identifier https://github.com/bitcoin/bitcoin/issues/29445
(https://github.com/bitcoin/bitcoin/pull/26817#issuecomment-1951977757)
I've opened a similar issue proposing to remove all of these inconsistent and hard to maintain headers and replace them with simple SPDX-License-Identifier https://github.com/bitcoin/bitcoin/issues/29445
💬 naumenkogs commented on pull request "net: attempts to connect to all resolved addresses when connecting to a node":
(https://github.com/bitcoin/bitcoin/pull/28834#discussion_r1494207563)
Yeah, after looking more into the current PR, it achieves nearly the same goal...
I'm fine with not doing anything here.
But it would be cool to understand better what it achieves :)
Here's what i think is going on. A DNS implementation may (or may not) return results in the same order, and then all the nodes will end up making connections with the same result. Then it's useful for load balancing, and to avoid DNS influencing the connectivity in general.
Might be worth adding this commen
...
(https://github.com/bitcoin/bitcoin/pull/28834#discussion_r1494207563)
Yeah, after looking more into the current PR, it achieves nearly the same goal...
I'm fine with not doing anything here.
But it would be cool to understand better what it achieves :)
Here's what i think is going on. A DNS implementation may (or may not) return results in the same order, and then all the nodes will end up making connections with the same result. Then it's useful for load balancing, and to avoid DNS influencing the connectivity in general.
Might be worth adding this commen
...