🤔 rkrux reviewed a pull request: "wallet, test: Replace MockableDatabase with in-memory SQLiteDatabase"
(https://github.com/bitcoin/bitcoin/pull/33032#pullrequestreview-3051716437)
Concept ACK f69852441e56c70edaebeaa7e2db4c6249c7e17c
I have looked in depth only the last commit. Almost all of my suggestions merge towards having child mock class(es) extending the base SQLite specific classes, wherever required. They can be deleted in the future if the need arises without touching the base SQLite classes. Also, I believe they would not make it difficult to achieve the following goal stated in the PR desc.
> This is particularly useful for future work that has the wallet
...
(https://github.com/bitcoin/bitcoin/pull/33032#pullrequestreview-3051716437)
Concept ACK f69852441e56c70edaebeaa7e2db4c6249c7e17c
I have looked in depth only the last commit. Almost all of my suggestions merge towards having child mock class(es) extending the base SQLite specific classes, wherever required. They can be deleted in the future if the need arises without touching the base SQLite classes. Also, I believe they would not make it difficult to achieve the following goal stated in the PR desc.
> This is particularly useful for future work that has the wallet
...
💬 rkrux commented on pull request "wallet, test: Replace MockableDatabase with in-memory SQLiteDatabase":
(https://github.com/bitcoin/bitcoin/pull/33032#discussion_r2228565019)
In https://github.com/bitcoin/bitcoin/commit/f69852441e56c70edaebeaa7e2db4c6249c7e17c "test, wallet: Replace MockableDatabase with in-memory SQLiteDatabase"
```diff
- return std::make_unique<SQLiteDatabase>(fs::PathFromString("mock/"), fs::PathFromString("mock/wallet.dat"), dummy_options, /*mock=*/true);
+ return std::make_unique<MockSQLiteDatabase>();
```
The `MockSQLiteDatabase` constructor can hardcode most of these mock values.
(https://github.com/bitcoin/bitcoin/pull/33032#discussion_r2228565019)
In https://github.com/bitcoin/bitcoin/commit/f69852441e56c70edaebeaa7e2db4c6249c7e17c "test, wallet: Replace MockableDatabase with in-memory SQLiteDatabase"
```diff
- return std::make_unique<SQLiteDatabase>(fs::PathFromString("mock/"), fs::PathFromString("mock/wallet.dat"), dummy_options, /*mock=*/true);
+ return std::make_unique<MockSQLiteDatabase>();
```
The `MockSQLiteDatabase` constructor can hardcode most of these mock values.
💬 rkrux commented on pull request "wallet, test: Replace MockableDatabase with in-memory SQLiteDatabase":
(https://github.com/bitcoin/bitcoin/pull/33032#discussion_r2228547723)
In f69852441e56c70edaebeaa7e2db4c6249c7e17c "test, wallet: Replace MockableDatabase with in-memory SQLiteDatabase"
Okay with removal of classes that extend the generic classes such as `WalletDatabase, DatabaseCursor, DatabaseBatch`, but I don't prefer seeing such comments in the non-test code. It's usually a sign of low cohesion in the design imho, which is not desirable.
An alternative could be to create a new class `MockSQLiteBatch` that extends `SQLiteBatch`. The test specific implement
...
(https://github.com/bitcoin/bitcoin/pull/33032#discussion_r2228547723)
In f69852441e56c70edaebeaa7e2db4c6249c7e17c "test, wallet: Replace MockableDatabase with in-memory SQLiteDatabase"
Okay with removal of classes that extend the generic classes such as `WalletDatabase, DatabaseCursor, DatabaseBatch`, but I don't prefer seeing such comments in the non-test code. It's usually a sign of low cohesion in the design imho, which is not desirable.
An alternative could be to create a new class `MockSQLiteBatch` that extends `SQLiteBatch`. The test specific implement
...
💬 rkrux commented on pull request "wallet, test: Replace MockableDatabase with in-memory SQLiteDatabase":
(https://github.com/bitcoin/bitcoin/pull/33032#discussion_r2228558213)
In https://github.com/bitcoin/bitcoin/commit/f69852441e56c70edaebeaa7e2db4c6249c7e17c "test, wallet: Replace MockableDatabase with in-memory SQLiteDatabase"
I see the `m_mock` was already present in `SQLiteDatabase` class but perhaps this PR can be an opportunity to take it out from it by extending this class by a `MockSQLiteDatabase`. `git grep -n m_mock` shows limited `if` checks in the non-test code that can be removed now.
(https://github.com/bitcoin/bitcoin/pull/33032#discussion_r2228558213)
In https://github.com/bitcoin/bitcoin/commit/f69852441e56c70edaebeaa7e2db4c6249c7e17c "test, wallet: Replace MockableDatabase with in-memory SQLiteDatabase"
I see the `m_mock` was already present in `SQLiteDatabase` class but perhaps this PR can be an opportunity to take it out from it by extending this class by a `MockSQLiteDatabase`. `git grep -n m_mock` shows limited `if` checks in the non-test code that can be removed now.
💬 rkrux commented on pull request "wallet, test: Replace MockableDatabase with in-memory SQLiteDatabase":
(https://github.com/bitcoin/bitcoin/pull/33032#discussion_r2228624749)
In https://github.com/bitcoin/bitcoin/commit/f69852441e56c70edaebeaa7e2db4c6249c7e17c "test, wallet: Replace MockableDatabase with in-memory SQLiteDatabase"
Checks like these can be avoided with the `MockSQLiteDatabase` class.
(https://github.com/bitcoin/bitcoin/pull/33032#discussion_r2228624749)
In https://github.com/bitcoin/bitcoin/commit/f69852441e56c70edaebeaa7e2db4c6249c7e17c "test, wallet: Replace MockableDatabase with in-memory SQLiteDatabase"
Checks like these can be avoided with the `MockSQLiteDatabase` class.
💬 rkrux commented on pull request "wallet, test: Replace MockableDatabase with in-memory SQLiteDatabase":
(https://github.com/bitcoin/bitcoin/pull/33032#discussion_r2228552486)
In https://github.com/bitcoin/bitcoin/commit/f69852441e56c70edaebeaa7e2db4c6249c7e17c "test, wallet: Replace MockableDatabase with in-memory SQLiteDatabase"
I don't think there should be mock related checks in the non-test code. A `MockSQLiteDatabase` class extending `SQLiteDatabase` class can override this function.
(https://github.com/bitcoin/bitcoin/pull/33032#discussion_r2228552486)
In https://github.com/bitcoin/bitcoin/commit/f69852441e56c70edaebeaa7e2db4c6249c7e17c "test, wallet: Replace MockableDatabase with in-memory SQLiteDatabase"
I don't think there should be mock related checks in the non-test code. A `MockSQLiteDatabase` class extending `SQLiteDatabase` class can override this function.
💬 rkrux commented on pull request "wallet, test: Replace MockableDatabase with in-memory SQLiteDatabase":
(https://github.com/bitcoin/bitcoin/pull/33032#discussion_r2228606629)
In https://github.com/bitcoin/bitcoin/commit/f69852441e56c70edaebeaa7e2db4c6249c7e17c "test, wallet: Replace MockableDatabase with in-memory SQLiteDatabase"
Nit and need not be done in this PR: To avoid hardcoding mocking constants in non-test code, perhaps the following? If the return value of `Format` had been an enum, the following would come across as a tighter check.
```diff
- Assert(m_database->Format() == "bdb_ro" || m_database->Format() == "sqlite-mock");
+ Assert(m_database->For
...
(https://github.com/bitcoin/bitcoin/pull/33032#discussion_r2228606629)
In https://github.com/bitcoin/bitcoin/commit/f69852441e56c70edaebeaa7e2db4c6249c7e17c "test, wallet: Replace MockableDatabase with in-memory SQLiteDatabase"
Nit and need not be done in this PR: To avoid hardcoding mocking constants in non-test code, perhaps the following? If the return value of `Format` had been an enum, the following would come across as a tighter check.
```diff
- Assert(m_database->Format() == "bdb_ro" || m_database->Format() == "sqlite-mock");
+ Assert(m_database->For
...
💬 ryanofsky commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#discussion_r2228654424)
re: https://github.com/bitcoin/bitcoin/pull/31802#discussion_r2228571627
> Yea, these need fleshing out [...]
I hope the release notes in #31679 (https://github.com/ryanofsky/bitcoin/blob/pr/libexec/doc/release-notes-31375-31679.md) can be a good starting point for a unified description of all the changes. I think all that might be needed there is a paragraph mentioning that the new `bitcoin` command has a *multiprocess* `-m` option enabling features which allow separate processes to commu
...
(https://github.com/bitcoin/bitcoin/pull/31802#discussion_r2228654424)
re: https://github.com/bitcoin/bitcoin/pull/31802#discussion_r2228571627
> Yea, these need fleshing out [...]
I hope the release notes in #31679 (https://github.com/ryanofsky/bitcoin/blob/pr/libexec/doc/release-notes-31375-31679.md) can be a good starting point for a unified description of all the changes. I think all that might be needed there is a paragraph mentioning that the new `bitcoin` command has a *multiprocess* `-m` option enabling features which allow separate processes to commu
...
💬 fanquake commented on pull request "Don't fix Python patch version":
(https://github.com/bitcoin/bitcoin/pull/33051#issuecomment-3113646151)
https://cirrus-ci.com/task/4540580631412736?logs=build#L1735:
```bash
[12:20:39.690] 38.29 ++ cat /.python-version
[12:20:39.690] 38.29 + env CC=clang python-build 3.10 /python_build
[12:20:39.690] 38.31 python-build: definition not found: 3.10
[12:20:39.690] ------
```
(https://github.com/bitcoin/bitcoin/pull/33051#issuecomment-3113646151)
https://cirrus-ci.com/task/4540580631412736?logs=build#L1735:
```bash
[12:20:39.690] 38.29 ++ cat /.python-version
[12:20:39.690] 38.29 + env CC=clang python-build 3.10 /python_build
[12:20:39.690] 38.31 python-build: definition not found: 3.10
[12:20:39.690] ------
```
💬 maflcko commented on pull request "wallet, test: Replace MockableDatabase with in-memory SQLiteDatabase":
(https://github.com/bitcoin/bitcoin/pull/33032#discussion_r2228661636)
nit in 78897500676f7ab1b197da56c0a676efcb49ec41 : I guess there is no real risk in having them public, due to the types involved, but if you wanted to make them protected, it could be done with this diff:
```diff
diff --git a/src/wallet/sqlite.h b/src/wallet/sqlite.h
index 57876bd83c..ce703f8f13 100644
--- a/src/wallet/sqlite.h
+++ b/src/wallet/sqlite.h
@@ -75,6 +75,14 @@ private:
void SetupSQLStatements();
bool ExecStatement(sqlite3_stmt* stmt, std::span<const std::byte> blo
...
(https://github.com/bitcoin/bitcoin/pull/33032#discussion_r2228661636)
nit in 78897500676f7ab1b197da56c0a676efcb49ec41 : I guess there is no real risk in having them public, due to the types involved, but if you wanted to make them protected, it could be done with this diff:
```diff
diff --git a/src/wallet/sqlite.h b/src/wallet/sqlite.h
index 57876bd83c..ce703f8f13 100644
--- a/src/wallet/sqlite.h
+++ b/src/wallet/sqlite.h
@@ -75,6 +75,14 @@ private:
void SetupSQLStatements();
bool ExecStatement(sqlite3_stmt* stmt, std::span<const std::byte> blo
...
💬 maflcko commented on pull request "log: [refactor] Use info level for init logs":
(https://github.com/bitcoin/bitcoin/pull/32967#discussion_r2228678313)
> instead of having to check if we consider the text a warning or an info as well (since there are many of remaining `LogPrintf` calls that I would still consider info logs but weren't migrated here for some reason.
The whole point of having log levels is that they are used correctly. Otherwise, if we don't care, we should just close this pull (and any related ones). If we do care, we should just review it (it isn't that hard tbh).
As for the scope of this pull: It is only about init logs
...
(https://github.com/bitcoin/bitcoin/pull/32967#discussion_r2228678313)
> instead of having to check if we consider the text a warning or an info as well (since there are many of remaining `LogPrintf` calls that I would still consider info logs but weren't migrated here for some reason.
The whole point of having log levels is that they are used correctly. Otherwise, if we don't care, we should just close this pull (and any related ones). If we do care, we should just review it (it isn't that hard tbh).
As for the scope of this pull: It is only about init logs
...
💬 Sjors commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-3113688256)
I went back to having `sudo apt-get install` in a single line, see https://github.com/bitcoin/bitcoin/pull/31802#discussion_r2228508989.
Squashed the Maintenance.cmake change into the commit that adds these to the guix build, see https://github.com/bitcoin/bitcoin/pull/31802#discussion_r2228510972.
Improved release notes, see https://github.com/bitcoin/bitcoin/pull/31802#discussion_r2161585663
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-3113688256)
I went back to having `sudo apt-get install` in a single line, see https://github.com/bitcoin/bitcoin/pull/31802#discussion_r2228508989.
Squashed the Maintenance.cmake change into the commit that adds these to the guix build, see https://github.com/bitcoin/bitcoin/pull/31802#discussion_r2228510972.
Improved release notes, see https://github.com/bitcoin/bitcoin/pull/31802#discussion_r2161585663
🤔 fanquake reviewed a pull request: "log: [refactor] Use info level for init logs"
(https://github.com/bitcoin/bitcoin/pull/32967#pullrequestreview-3051929630)
ACK 26a43ed0cf6577bfa6e60b979179b0d99c2ad7db
(https://github.com/bitcoin/bitcoin/pull/32967#pullrequestreview-3051929630)
ACK 26a43ed0cf6577bfa6e60b979179b0d99c2ad7db
📝 raul-anton-2005 opened a pull request: "doc: Improve comments in FindCoins function for clarity"
(https://github.com/bitcoin/bitcoin/pull/33053)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
(https://github.com/bitcoin/bitcoin/pull/33053)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
💬 maflcko commented on pull request "log: [refactor] Use info level for init logs":
(https://github.com/bitcoin/bitcoin/pull/32967#discussion_r2228717203)
thx, done. (Did a rebase and then a zero-diff to split into two commits)
(https://github.com/bitcoin/bitcoin/pull/32967#discussion_r2228717203)
thx, done. (Did a rebase and then a zero-diff to split into two commits)
✅ maflcko closed a pull request: "doc: Improve comments in FindCoins function for clarity"
(https://github.com/bitcoin/bitcoin/pull/33053)
(https://github.com/bitcoin/bitcoin/pull/33053)
💬 maflcko commented on pull request "doc: Improve comments in FindCoins function for clarity":
(https://github.com/bitcoin/bitcoin/pull/33053#issuecomment-3113745311)
Thanks, but there is no need to add LLM generated docs to self-explanatory code.
(https://github.com/bitcoin/bitcoin/pull/33053#issuecomment-3113745311)
Thanks, but there is no need to add LLM generated docs to self-explanatory code.
💬 Sjors commented on pull request "Don't fix Python patch version":
(https://github.com/bitcoin/bitcoin/pull/33051#issuecomment-3113814862)
Ah I see. For some reason the linter is calling `python-build` directly instead of `pyenv install`, which just picks a patch version. Added a workaround.
(https://github.com/bitcoin/bitcoin/pull/33051#issuecomment-3113814862)
Ah I see. For some reason the linter is calling `python-build` directly instead of `pyenv install`, which just picks a patch version. Added a workaround.
👍 ryanofsky approved a pull request: "Add bitcoin-{node,gui} to release binaries for IPC"
(https://github.com/bitcoin/bitcoin/pull/31802#pullrequestreview-3051962961)
Code review ACK 082b416a1bcbfbbdcafb3a3eb1ae800c93385203. Since last review: squashed commit, updated release notes and reverted ci.yml reformatting.
Maybe it's just me, but I feel like this PR is more comprehensible now that the squashed depends commit is first. And the new release notes are also much better. (Though I would still like to unify them later with the bitcoin wrapper & libexec notes as suggested https://github.com/bitcoin/bitcoin/pull/31802#discussion_r2228654424).
> I could
...
(https://github.com/bitcoin/bitcoin/pull/31802#pullrequestreview-3051962961)
Code review ACK 082b416a1bcbfbbdcafb3a3eb1ae800c93385203. Since last review: squashed commit, updated release notes and reverted ci.yml reformatting.
Maybe it's just me, but I feel like this PR is more comprehensible now that the squashed depends commit is first. And the new release notes are also much better. (Though I would still like to unify them later with the bitcoin wrapper & libexec notes as suggested https://github.com/bitcoin/bitcoin/pull/31802#discussion_r2228654424).
> I could
...
💬 ryanofsky commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#discussion_r2228720121)
In commit "build: add bitcoin-{node,gui} to Maintenance.cmake" (362e233da72b89723cdb9f4f56342716b60976eb)
Note for reviewers: Since this PR was opened, full windows support (for both connecting to unix sockets and spawning subprocesses) was implemented in #32387. #32387 is a draft just because I don't want it to be a review burden while there are other changes like #32345 that I think are more important.
(https://github.com/bitcoin/bitcoin/pull/31802#discussion_r2228720121)
In commit "build: add bitcoin-{node,gui} to Maintenance.cmake" (362e233da72b89723cdb9f4f56342716b60976eb)
Note for reviewers: Since this PR was opened, full windows support (for both connecting to unix sockets and spawning subprocesses) was implemented in #32387. #32387 is a draft just because I don't want it to be a review burden while there are other changes like #32345 that I think are more important.