✅ 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?
💬 hodlinator commented on pull request "bench: add support for custom data directory":
(https://github.com/bitcoin/bitcoin/pull/31000#discussion_r1788722245)
> I don't feel strong about the capitalization neither but it looks incorrect to introduce it now when all other functions aren't using it.
(`SetupBenchArgs` is capitalized, `main` must be non-capitalized, so that only leaves `parseAsymptote` to change beyond the function under discussion).
(https://github.com/bitcoin/bitcoin/pull/31000#discussion_r1788722245)
> I don't feel strong about the capitalization neither but it looks incorrect to introduce it now when all other functions aren't using it.
(`SetupBenchArgs` is capitalized, `main` must be non-capitalized, so that only leaves `parseAsymptote` to change beyond the function under discussion).