💬 glozow commented on pull request "package validation: relax the package-not-child-with-unconfirmed-parents rule":
(https://github.com/bitcoin/bitcoin/pull/31385#discussion_r2228571572)
ah doi, restored
(https://github.com/bitcoin/bitcoin/pull/31385#discussion_r2228571572)
ah doi, restored
💬 glozow commented on pull request "package validation: relax the package-not-child-with-unconfirmed-parents rule":
(https://github.com/bitcoin/bitcoin/pull/31385#discussion_r2228570611)
🤦 thank you, fixed
(https://github.com/bitcoin/bitcoin/pull/31385#discussion_r2228570611)
🤦 thank you, fixed
💬 Sjors commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#discussion_r2228578817)
Fixed the sort
(https://github.com/bitcoin/bitcoin/pull/31802#discussion_r2228578817)
Fixed the sort
💬 fanquake commented on pull request "[29.x] backport #32069":
(https://github.com/bitcoin/bitcoin/pull/33052#issuecomment-3113551574)
Thanks, this should have been backported when #31757 was done, but likely missed at that happened later. Can you also add the rel note: https://github.com/fanquake/bitcoin/commit/da24253775f9f6cecd8fbcf65e8aa6f40090f72c.
(https://github.com/bitcoin/bitcoin/pull/33052#issuecomment-3113551574)
Thanks, this should have been backported when #31757 was done, but likely missed at that happened later. Can you also add the rel note: https://github.com/fanquake/bitcoin/commit/da24253775f9f6cecd8fbcf65e8aa6f40090f72c.
💬 fanquake commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#discussion_r2228510972)
In 28af4d727b3eeff3da0a3dfab0260c57fb52111c: This should be squashed into the commit that actually changes the Guix build. Ideally CMake would warn/error here, at least in the Guix build. We don't want it to continue silently if binaries don't exist/are missing from these checks.
(https://github.com/bitcoin/bitcoin/pull/31802#discussion_r2228510972)
In 28af4d727b3eeff3da0a3dfab0260c57fb52111c: This should be squashed into the commit that actually changes the Guix build. Ideally CMake would warn/error here, at least in the Guix build. We don't want it to continue silently if binaries don't exist/are missing from these checks.
💬 fanquake commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#discussion_r2228508989)
Please don't make random (incomplete codebase-wise) formatting changes, to unrelated parts of the code. I think there's enough happening in this PR.
(https://github.com/bitcoin/bitcoin/pull/31802#discussion_r2228508989)
Please don't make random (incomplete codebase-wise) formatting changes, to unrelated parts of the code. I think there's enough happening in this PR.
💬 fanquake commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#discussion_r2228571627)
Yea, these need fleshing out, because they don't give much useful information to users, and will likely leave them with questions: "What's IPC?" (never defined), "What's `bitcoin-node`; should I use that instead of `bitcoind`?" (same for the GUI), "Why is this enabled by default if it's for some experimental feature I (and basically all other users) don't use?".
(https://github.com/bitcoin/bitcoin/pull/31802#discussion_r2228571627)
Yea, these need fleshing out, because they don't give much useful information to users, and will likely leave them with questions: "What's IPC?" (never defined), "What's `bitcoin-node`; should I use that instead of `bitcoind`?" (same for the GUI), "Why is this enabled by default if it's for some experimental feature I (and basically all other users) don't use?".
💬 Sjors commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#discussion_r2228590806)
This kept triggering needs-rebase, so seemed useful to fix. But I can revert.
(https://github.com/bitcoin/bitcoin/pull/31802#discussion_r2228590806)
This kept triggering needs-rebase, so seemed useful to fix. But I can revert.
💬 fanquake commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-3113566277)
You could probably split out the renaming, and just get them landed, given that should happen regardless of all the other changes here.
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-3113566277)
You could probably split out the renaming, and just get them landed, given that should happen regardless of all the other changes here.
🤔 pablomartin4btc reviewed a pull request: "rpc: refactor: use string_view in Arg/MaybeArg"
(https://github.com/bitcoin/bitcoin/pull/32983#pullrequestreview-3051797902)
re-ACK 122b432b71e148cd85edcd74df741f99ac7f187c
Not blocking: I’d personally prefer keeping `Arg<std::string>` in `signmessage.cpp` if we're not planning to update `MessageVerify` to accept `string_view` instead of `const std::string&`. Is updating `MessageVerify` something you’d prefer to avoid at this stage?
(https://github.com/bitcoin/bitcoin/pull/32983#pullrequestreview-3051797902)
re-ACK 122b432b71e148cd85edcd74df741f99ac7f187c
Not blocking: I’d personally prefer keeping `Arg<std::string>` in `signmessage.cpp` if we're not planning to update `MessageVerify` to accept `string_view` instead of `const std::string&`. Is updating `MessageVerify` something you’d prefer to avoid at this stage?
💬 darosior commented on pull request "[29.x] backport #32069":
(https://github.com/bitcoin/bitcoin/pull/33052#issuecomment-3113596396)
Sure, done.
(https://github.com/bitcoin/bitcoin/pull/33052#issuecomment-3113596396)
Sure, done.
💬 stickies-v commented on pull request "rpc: refactor: use string_view in Arg/MaybeArg":
(https://github.com/bitcoin/bitcoin/pull/32983#issuecomment-3113627621)
> Is updating `MessageVerify` something you’d prefer to avoid at this stage?
Yes. It involves a cascade of changes, many touching consensus code, that require significant review and have mostly nothing to do with RPC, so it would blow up the scope of this PR.
(https://github.com/bitcoin/bitcoin/pull/32983#issuecomment-3113627621)
> Is updating `MessageVerify` something you’d prefer to avoid at this stage?
Yes. It involves a cascade of changes, many touching consensus code, that require significant review and have mostly nothing to do with RPC, so it would blow up the scope of this PR.
🤔 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
...