⚠️ theStack opened an issue: "depends: `touch` command for creating determinstic archive timestamps fails on OpenBSD"
(https://github.com/bitcoin/bitcoin/issues/29447)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
In the course of the "Caching..." step of a depends build, the `touch` command for creating determinstic package archive timestamps fails on OpenBSD. The reason is that the used option `-h` option (introduced in PR #21995, commit 6ebe57622cb70df529879b15f291166177f2827c) doesn't exist in OpenBSD's version of touch (see https://man.openbsd.org/touch.1). E.g. for zeromq:
```
[ ... ]
Postp
...
(https://github.com/bitcoin/bitcoin/issues/29447)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
In the course of the "Caching..." step of a depends build, the `touch` command for creating determinstic package archive timestamps fails on OpenBSD. The reason is that the used option `-h` option (introduced in PR #21995, commit 6ebe57622cb70df529879b15f291166177f2827c) doesn't exist in OpenBSD's version of touch (see https://man.openbsd.org/touch.1). E.g. for zeromq:
```
[ ... ]
Postp
...
💬 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