💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2395115546)
All non-determinism issues have been resolved.
Guix build hashes for both `aarch64` and `x86_64`:
```
5214a1585050ee8504bf20db8bc70bc8e0f7282ef8eb453290ab8e909b32bd83 guix-build-27f3847d9f97/output/aarch64-linux-gnu/SHA256SUMS.part
b714cadb11d1250f7f5f27ed9d0d1ad10550e279de4f15e40c9008392b5189c3 guix-build-27f3847d9f97/output/aarch64-linux-gnu/bitcoin-27f3847d9f97-aarch64-linux-gnu-debug.tar.gz
06d41ffcf080d583e0cfb41c55286300e0f36d05d32f546177b54cb8d3e7531f guix-build-27f3847d9f97/out
...
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2395115546)
All non-determinism issues have been resolved.
Guix build hashes for both `aarch64` and `x86_64`:
```
5214a1585050ee8504bf20db8bc70bc8e0f7282ef8eb453290ab8e909b32bd83 guix-build-27f3847d9f97/output/aarch64-linux-gnu/SHA256SUMS.part
b714cadb11d1250f7f5f27ed9d0d1ad10550e279de4f15e40c9008392b5189c3 guix-build-27f3847d9f97/output/aarch64-linux-gnu/bitcoin-27f3847d9f97-aarch64-linux-gnu-debug.tar.gz
06d41ffcf080d583e0cfb41c55286300e0f36d05d32f546177b54cb8d3e7531f guix-build-27f3847d9f97/out
...
✅ Sjors closed an issue: "rfc: DATUM mining interface requirements"
(https://github.com/bitcoin/bitcoin/issues/31002)
(https://github.com/bitcoin/bitcoin/issues/31002)
📝 tdb3 opened a pull request: "test: enhance p2p_orphan_handling"
(https://github.com/bitcoin/bitcoin/pull/31037)
Previously, `p2p_orphan_handling` relied on checking the debug log for orphanage changes. This updates the tests to reduce debug log checking and add additional checks using `tx_in_orphanage()` and `getorphantxs` introduced in #30793.
Leaving as draft for now, since I'd like to review `p2p_orphan_handling` again to identify any other useful enhancements. Suggestions welcome.
(https://github.com/bitcoin/bitcoin/pull/31037)
Previously, `p2p_orphan_handling` relied on checking the debug log for orphanage changes. This updates the tests to reduce debug log checking and add additional checks using `tx_in_orphanage()` and `getorphantxs` introduced in #30793.
Leaving as draft for now, since I'd like to review `p2p_orphan_handling` again to identify any other useful enhancements. Suggestions welcome.
🤔 naiyoma reviewed a pull request: "ci: set a ctest test timeout of 1200 (20 minutes)"
(https://github.com/bitcoin/bitcoin/pull/31026#pullrequestreview-2349899770)
ACK [https://github.com/bitcoin/bitcoin/pull/31026/commits/93dda4c70254750a5b5c0e7f7d2d89032519b999](https://github.com/bitcoin/bitcoin/pull/31026/commits/93dda4c70254750a5b5c0e7f7d2d89032519b999)
(https://github.com/bitcoin/bitcoin/pull/31026#pullrequestreview-2349899770)
ACK [https://github.com/bitcoin/bitcoin/pull/31026/commits/93dda4c70254750a5b5c0e7f7d2d89032519b999](https://github.com/bitcoin/bitcoin/pull/31026/commits/93dda4c70254750a5b5c0e7f7d2d89032519b999)
💬 1440000bytes commented on pull request "[27.x] Even more backports":
(https://github.com/bitcoin/bitcoin/pull/30558#discussion_r1788656362)
Its weird that it was added in release notes for 27.2 but not 28.0.
Anyway, it doesn't really make any difference. Just shows the lack of courtesy.
Cc: @jonatack
(https://github.com/bitcoin/bitcoin/pull/30558#discussion_r1788656362)
Its weird that it was added in release notes for 27.2 but not 28.0.
Anyway, it doesn't really make any difference. Just shows the lack of courtesy.
Cc: @jonatack
💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1785530379)
added
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1785530379)
added
💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1785527518)
edited
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1785527518)
edited
💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1788656263)
fixed
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1788656263)
fixed
💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1788656274)
fixed, thanks
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1788656274)
fixed, thanks
💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1788656312)
done
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1788656312)
done
💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1788656318)
done
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1788656318)
done
💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1788656284)
fixed
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1788656284)
fixed
💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1785529982)
added
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1785529982)
added
💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1788656296)
fixed
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1788656296)
fixed
💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1788656350)
Accidental, I think. changed back
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1788656350)
Accidental, I think. changed back
💬 hodlinator commented on pull request "descriptor: Add proper Clone function to miniscript::Node":
(https://github.com/bitcoin/bitcoin/pull/30866#discussion_r1788683926)
How come the shallow copy in the version before the PR is causing a problem in this case though?
It seems the default-generated copy-ctor for `Node` used before was somehow ending up with corrupt/leaked data, but I've been unable to spot what it is. Can't see any slicing going on. Is something funky being done to the `shared_ptr`s?
Regardless, it might be worth adding:
```
Node(const Node&) = delete;
Node(Node&&) = delete;
Node& operator=(const Node&) = delete;
Node& o
...
(https://github.com/bitcoin/bitcoin/pull/30866#discussion_r1788683926)
How come the shallow copy in the version before the PR is causing a problem in this case though?
It seems the default-generated copy-ctor for `Node` used before was somehow ending up with corrupt/leaked data, but I've been unable to spot what it is. Can't see any slicing going on. Is something funky being done to the `shared_ptr`s?
Regardless, it might be worth adding:
```
Node(const Node&) = delete;
Node(Node&&) = delete;
Node& operator=(const Node&) = delete;
Node& o
...
💬 hodlinator commented on issue "RFC: Formal description of the RPC API":
(https://github.com/bitcoin/bitcoin/issues/29912#issuecomment-2395195267)
Having a formal spec, either [generated similarly to the current RPC docs](https://github.com/bitcoin-core/bitcoincore.org/blob/master/contrib/doc-gen/generate.go) (or possibly used to generate stub headers which are then implemented in Bitcoin Core) lowers friction for application developers building on top of Bitcoin Core and reduces temptation to build on top of other projects (including non-Bitcoin ones).
Been digging into OpenRPC a bit. It seems to be used primarily by Ethereum and Ether
...
(https://github.com/bitcoin/bitcoin/issues/29912#issuecomment-2395195267)
Having a formal spec, either [generated similarly to the current RPC docs](https://github.com/bitcoin-core/bitcoincore.org/blob/master/contrib/doc-gen/generate.go) (or possibly used to generate stub headers which are then implemented in Bitcoin Core) lowers friction for application developers building on top of Bitcoin Core and reduces temptation to build on top of other projects (including non-Bitcoin ones).
Been digging into OpenRPC a bit. It seems to be used primarily by Ethereum and Ether
...
📝 hodlinator opened a pull request: "test: Fix copy-paste in wallet/test/db_tests ostream operator"
(https://github.com/bitcoin/bitcoin/pull/31038)
Fix accidentally remaining copy-pasted variable name.
Example output when intentionally adding `expected.erase(expected.begin());` before `BOOST_CHECK_EQUAL_COLLECTIONS` in *db_tests.cpp*/`CheckPrefix`
Before fix:
```
src/wallet/test/db_tests.cpp(61): error: in "db_tests/db_cursor_prefix_byte_test": check { actual.begin(), actual.end() } == { expected.begin(), expected.end() } has failed.
Mismatch at position 0: ("�", "�") != ("�suffix", "�suffix")
Mismatch at position 1: ("�suffix",
...
(https://github.com/bitcoin/bitcoin/pull/31038)
Fix accidentally remaining copy-pasted variable name.
Example output when intentionally adding `expected.erase(expected.begin());` before `BOOST_CHECK_EQUAL_COLLECTIONS` in *db_tests.cpp*/`CheckPrefix`
Before fix:
```
src/wallet/test/db_tests.cpp(61): error: in "db_tests/db_cursor_prefix_byte_test": check { actual.begin(), actual.end() } == { expected.begin(), expected.end() } has failed.
Mismatch at position 0: ("�", "�") != ("�suffix", "�suffix")
Mismatch at position 1: ("�suffix",
...
🤔 hodlinator reviewed a pull request: "bench: add support for custom data directory"
(https://github.com/bitcoin/bitcoin/pull/31000#pullrequestreview-2350404241)
`git range-diff master 1ba225c ded1a6c`
Still feel like it would be good to call a common function adding *-testdatadir* to an `ArgsManager` instead of duplicating the functionality between test/bench (and having a different description-string).
(https://github.com/bitcoin/bitcoin/pull/31000#pullrequestreview-2350404241)
`git range-diff master 1ba225c ded1a6c`
Still feel like it would be good to call a common function adding *-testdatadir* to an `ArgsManager` instead of duplicating the functionality between test/bench (and having a different description-string).
💬 hodlinator commented on pull request "bench: add support for custom data directory":
(https://github.com/bitcoin/bitcoin/pull/31000#discussion_r1788721681)
Agree a `SetupTestArgs` may not communicate clearly enough that the function is used in a broader context. How about calling it `SetupCommonTestArgs` and adding a comment about it being used in benchmarks as well? That, together with no longer marking the function `static` (private to the compilation unit), might mitigate your concerns?
(https://github.com/bitcoin/bitcoin/pull/31000#discussion_r1788721681)
Agree a `SetupTestArgs` may not communicate clearly enough that the function is used in a broader context. How about calling it `SetupCommonTestArgs` and adding a comment about it being used in benchmarks as well? That, together with no longer marking the function `static` (private to the compilation unit), might mitigate your concerns?