💬 mzumsande commented on pull request "index: improve initialization and pruning violation check":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1199268910)
I don't think this is possible to trigger with the way the function is used, but if `upper_block` already has `NO_DATA`, it would return that block, maybe `nullptr` would be more natural? (also applies to `GetFirstStoredBlock`).
Also, the name sounds like a boolean function, maybe something like `CheckBlockDataAvailability()` would be better?
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1199268910)
I don't think this is possible to trigger with the way the function is used, but if `upper_block` already has `NO_DATA`, it would return that block, maybe `nullptr` would be more natural? (also applies to `GetFirstStoredBlock`).
Also, the name sounds like a boolean function, maybe something like `CheckBlockDataAvailability()` would be better?
💬 jarolrod commented on issue "crash on macOS 12.6.5":
(https://github.com/bitcoin-core/gui/issues/731#issuecomment-1555287731)
@ddykeman1 would you be able to compile bitcoin core on your own, enabling debug during the configure stage, that would greatly help to identify exactly what is going on.
I'd also suggest sanity checking your setup:
1) Is the data directory existing and available
2) Does the crash occur if you choose a different data directory? (You can run the gui on startup with `-resetguisettings` to be able to choose a new data directory within the gui itself on startup, or pass `-datadir` when running
...
(https://github.com/bitcoin-core/gui/issues/731#issuecomment-1555287731)
@ddykeman1 would you be able to compile bitcoin core on your own, enabling debug during the configure stage, that would greatly help to identify exactly what is going on.
I'd also suggest sanity checking your setup:
1) Is the data directory existing and available
2) Does the crash occur if you choose a different data directory? (You can run the gui on startup with `-resetguisettings` to be able to choose a new data directory within the gui itself on startup, or pass `-datadir` when running
...
⚠️ bolaubimerah opened an issue: "DEMO SLOT PRAGMATIC > Kumpulan Link Main Gratis Game Terlengkap SLOT DEMO Pragmatic Anti Lag Seperti Asli "
(https://github.com/bitcoin/bitcoin/issues/27704)
# DEMO SLOT PRAGMATIC > Kumpulan Link Main Gratis Game Terlengkap Akun SLOT DEMO Pragmatic Anti Lag Seperti Asli
# **[➡️ DAFTAR GRATIS AKUN DEMO SLOT PRAGMATIC](https://rebrand.ly/daftar-slot-gacor-pay4d)**
# **[➡️ LINK PRAGMATIC SLOT DEMO GRATIS](https://rebrand.ly/daftar-slot-gacor-pay4d)**
[Demo Slot Pragmatic](https://rebrand.ly/daftar-slot-gacor-pay4d) merupakan salah satu dari Demo Slot Online Terpercaya di Indonesia yang mampu memberikan berbagai macam kumpulan link game slot Demo
...
(https://github.com/bitcoin/bitcoin/issues/27704)
# DEMO SLOT PRAGMATIC > Kumpulan Link Main Gratis Game Terlengkap Akun SLOT DEMO Pragmatic Anti Lag Seperti Asli
# **[➡️ DAFTAR GRATIS AKUN DEMO SLOT PRAGMATIC](https://rebrand.ly/daftar-slot-gacor-pay4d)**
# **[➡️ LINK PRAGMATIC SLOT DEMO GRATIS](https://rebrand.ly/daftar-slot-gacor-pay4d)**
[Demo Slot Pragmatic](https://rebrand.ly/daftar-slot-gacor-pay4d) merupakan salah satu dari Demo Slot Online Terpercaya di Indonesia yang mampu memberikan berbagai macam kumpulan link game slot Demo
...
🤔 jarolrod reviewed a pull request: "doc: remove Security section from build-unix.md"
(https://github.com/bitcoin/bitcoin/pull/27688#pullrequestreview-1435213179)
ACK 4bfcbbfd4a9749f7954e37a7447b5f047465bdf8
(https://github.com/bitcoin/bitcoin/pull/27688#pullrequestreview-1435213179)
ACK 4bfcbbfd4a9749f7954e37a7447b5f047465bdf8
✅ achow101 closed an issue: "."
(https://github.com/bitcoin/bitcoin/issues/27704)
(https://github.com/bitcoin/bitcoin/issues/27704)
:lock: achow101 locked an issue: "."
(https://github.com/bitcoin/bitcoin/issues/27704)
(https://github.com/bitcoin/bitcoin/issues/27704)
👍 jarolrod approved a pull request: "Wallet : Allow user to navigate options while encrypting at creation"
(https://github.com/bitcoin-core/gui/pull/722#pullrequestreview-1435242481)
ACK 78660e72001a2561c7ad3026367a69f65414dbd9
Tested for functionality and sanity checked the user flows between master and this PR.
(https://github.com/bitcoin-core/gui/pull/722#pullrequestreview-1435242481)
ACK 78660e72001a2561c7ad3026367a69f65414dbd9
Tested for functionality and sanity checked the user flows between master and this PR.
💬 ryanofsky commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1198931499)
In commit "test: add coverage for db cursor prefix range iteration" (83b4e50291a0bd2c45f370cb18bb479d8f73bc71)
I think you need to add a case like `"\xff\xff"` to test sqlite code without a `key < end_range` condition
Would also consider squashing this commit into earlier commit "wallet: Add GetPrefixCursor to DatabaseBatch" (57f35c90d2ebbc657a781ba3918398720b4319eb) because it's helpful to see test code calling a new function in the same commit that introduces the function.
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1198931499)
In commit "test: add coverage for db cursor prefix range iteration" (83b4e50291a0bd2c45f370cb18bb479d8f73bc71)
I think you need to add a case like `"\xff\xff"` to test sqlite code without a `key < end_range` condition
Would also consider squashing this commit into earlier commit "wallet: Add GetPrefixCursor to DatabaseBatch" (57f35c90d2ebbc657a781ba3918398720b4319eb) because it's helpful to see test code calling a new function in the same commit that introduces the function.
🤔 ryanofsky reviewed a pull request: "wallet: Load database records in a particular order"
(https://github.com/bitcoin/bitcoin/pull/24914#pullrequestreview-1431487191)
This looks great so far! Here is where I am in the review:
- [X] 79075f970802031d42c3808351baa91d5edf9783 wallet: Add GetPrefixCursor to DatabaseBatch (1/18)
- [X] 37a3e64ec84899468aab8c55fd556c93d195e322 walletdb: Consistently clear key and value streams before writing (2/18)
- [X] 39bfce498a7576cd41b2b362ee7688b665472f33 test: add coverage for db cursor prefix range iteration (3/18)
- [X] 3fbbdb2925a2e65b75af9116773e059dd5453afa walletdb: Refactor minversion loading (4/18)
- [X] bc2d888
...
(https://github.com/bitcoin/bitcoin/pull/24914#pullrequestreview-1431487191)
This looks great so far! Here is where I am in the review:
- [X] 79075f970802031d42c3808351baa91d5edf9783 wallet: Add GetPrefixCursor to DatabaseBatch (1/18)
- [X] 37a3e64ec84899468aab8c55fd556c93d195e322 walletdb: Consistently clear key and value streams before writing (2/18)
- [X] 39bfce498a7576cd41b2b362ee7688b665472f33 test: add coverage for db cursor prefix range iteration (3/18)
- [X] 3fbbdb2925a2e65b75af9116773e059dd5453afa walletdb: Refactor minversion loading (4/18)
- [X] bc2d888
...
💬 ryanofsky commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1198953172)
In commit "walletdb: Refactor key reading and loading to its own function" (e048fe71038caddb49a5875727a14e05f6edb0d3)
Note: there is a slight change in behavior this commit which could be noted in the commit message.
If this deserialization fails, or if any error conditions are hit below, `wss.nKeys++` will not be incremented anymore. This seems ok, but previously `wss.nKeys++` included the total number of keys found, even ones that could not be loaded.
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1198953172)
In commit "walletdb: Refactor key reading and loading to its own function" (e048fe71038caddb49a5875727a14e05f6edb0d3)
Note: there is a slight change in behavior this commit which could be noted in the commit message.
If this deserialization fails, or if any error conditions are hit below, `wss.nKeys++` will not be incremented anymore. This seems ok, but previously `wss.nKeys++` included the total number of keys found, even ones that could not be loaded.
💬 ryanofsky commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1198965953)
In commit "salvage: Remove use of ReadKeyValue in salvage" (c48aa9d0df7e34493a666c55e060304ef262d4b9)
No need to move KeyFilterFn here, can just drop it entirely:
```diff
diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp
index 5d3ced0ad225..6ab29379fb26 100644
--- a/src/wallet/walletdb.cpp
+++ b/src/wallet/walletdb.cpp
@@ -461,22 +461,15 @@ bool LoadHDChain(CWallet* pwallet, CDataStream& ssValue)
return true;
}
-//! Callback for filtering key types to deserial
...
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1198965953)
In commit "salvage: Remove use of ReadKeyValue in salvage" (c48aa9d0df7e34493a666c55e060304ef262d4b9)
No need to move KeyFilterFn here, can just drop it entirely:
```diff
diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp
index 5d3ced0ad225..6ab29379fb26 100644
--- a/src/wallet/walletdb.cpp
+++ b/src/wallet/walletdb.cpp
@@ -461,22 +461,15 @@ bool LoadHDChain(CWallet* pwallet, CDataStream& ssValue)
return true;
}
-//! Callback for filtering key types to deserial
...
💬 ryanofsky commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1199487534)
In commit "walletdb: Refactor legacy wallet record loading into its own function" (8648511b567dfcdea7ffa5ac4595a43a768f5525)
What's the reason for using (0, 0) flags here and other places instead of (SER_DISK, CLIENT_VERSION) flags used elsewhere? Having a code comment about this somewhere would be very helpful for understanding. It seems a little dodgy to get rid of SER_DISK flag if present or future code might rely on it.
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1199487534)
In commit "walletdb: Refactor legacy wallet record loading into its own function" (8648511b567dfcdea7ffa5ac4595a43a768f5525)
What's the reason for using (0, 0) flags here and other places instead of (SER_DISK, CLIENT_VERSION) flags used elsewhere? Having a code comment about this somewhere would be very helpful for understanding. It seems a little dodgy to get rid of SER_DISK flag if present or future code might rely on it.
💬 ryanofsky commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1198934920)
In commit "test: add coverage for db cursor prefix range iteration" (83b4e50291a0bd2c45f370cb18bb479d8f73bc71)
Setting this flag seems unnecessary and unrelated to the test
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1198934920)
In commit "test: add coverage for db cursor prefix range iteration" (83b4e50291a0bd2c45f370cb18bb479d8f73bc71)
Setting this flag seems unnecessary and unrelated to the test
💬 ryanofsky commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1198957038)
In commit "walletdb: Refactor crypted key loading to its own function" (59ac3bccffe3fda352dc068fa3a0b507ed9a73d6)
Similar to previous commit, now `wss.nCKeys` will not be incremented if this fails. Change in behavior could be noted in commit message.
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1198957038)
In commit "walletdb: Refactor crypted key loading to its own function" (59ac3bccffe3fda352dc068fa3a0b507ed9a73d6)
Similar to previous commit, now `wss.nCKeys` will not be incremented if this fails. Change in behavior could be noted in commit message.
💬 ryanofsky commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1199488667)
In commit "walletdb: Refactor legacy wallet record loading into its own function" (8648511b567dfcdea7ffa5ac4595a43a768f5525)
Would be helpful to have a code comment here saying enum number values are significant because if there are different error codes from reading different rows of the database, the error code with the highest number is the one that will be returned.
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1199488667)
In commit "walletdb: Refactor legacy wallet record loading into its own function" (8648511b567dfcdea7ffa5ac4595a43a768f5525)
Would be helpful to have a code comment here saying enum number values are significant because if there are different error codes from reading different rows of the database, the error code with the highest number is the one that will be returned.
💬 ryanofsky commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1199490724)
In commit "walletdb: Refactor legacy wallet record loading into its own function" (8648511b567dfcdea7ffa5ac4595a43a768f5525)
It looks like in this commit, the side-effect in previous commit "walletdb: Refactor key reading and loading to its own function" (e048fe71038caddb49a5875727a14e05f6edb0d3) of changing the `wss.nKeys` value in case of key errors is partially reverted, because now the number of keys is incremented unconditionally, while the previous commit added more conditions.
Woul
...
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1199490724)
In commit "walletdb: Refactor legacy wallet record loading into its own function" (8648511b567dfcdea7ffa5ac4595a43a768f5525)
It looks like in this commit, the side-effect in previous commit "walletdb: Refactor key reading and loading to its own function" (e048fe71038caddb49a5875727a14e05f6edb0d3) of changing the `wss.nKeys` value in case of key errors is partially reverted, because now the number of keys is incremented unconditionally, while the previous commit added more conditions.
Woul
...
💬 ryanofsky commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1199493269)
re: https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1102044608
In commit "walletdb: Refactor legacy wallet record loading into its own function" (8648511b567dfcdea7ffa5ac4595a43a768f5525)
> We print all corrupt records of one type, not all the corrupted records
I guess I disagree a little, but I think it is good to log errors about everything we know is corrupt, even if we don't log errors about things we can't know are corrupt.
Regardless, if there is a change in behavior
...
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1199493269)
re: https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1102044608
In commit "walletdb: Refactor legacy wallet record loading into its own function" (8648511b567dfcdea7ffa5ac4595a43a768f5525)
> We print all corrupt records of one type, not all the corrupted records
I guess I disagree a little, but I think it is good to log errors about everything we know is corrupt, even if we don't log errors about things we can't know are corrupt.
Regardless, if there is a change in behavior
...
💬 achow101 commented on pull request "rpc: add `descriptorprocesspsbt` rpc":
(https://github.com/bitcoin/bitcoin/pull/25796#issuecomment-1555399958)
re-ACK 1bce12acd3e271a7c88d9400b4e3a5645bc8a911
(https://github.com/bitcoin/bitcoin/pull/25796#issuecomment-1555399958)
re-ACK 1bce12acd3e271a7c88d9400b4e3a5645bc8a911
💬 hebasto commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1199572071)
> but is there a bug?
No, there is not. I've double-checked the code, it looks OK for me too.
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1199572071)
> but is there a bug?
No, there is not. I've double-checked the code, it looks OK for me too.
💬 hebasto commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#issuecomment-1555785546)
Reviewing the code:
- [x] 974b4293a5f2fe01302c608bf0063e3b52721d0b "kernel: Add notification interface"
- [x] ec5043d8c120332c0bf916b5b576aff8374af1ed "kernel: Add headerTip method to notifications"
- [ ] 1be1c16b70edc5def98eb1903cee08b5d546ba88 "kernel: Add progress method to notifications"
- [ ] 483fe64956179872e5821483474c52d955949e40 "kernel: Add warning method to notifications"
- [ ] 3785d38417618e921c5233cf14aa3c802f37387b "refactor: Move ScheduleBatchPriority to its own file"
- [
...
(https://github.com/bitcoin/bitcoin/pull/27636#issuecomment-1555785546)
Reviewing the code:
- [x] 974b4293a5f2fe01302c608bf0063e3b52721d0b "kernel: Add notification interface"
- [x] ec5043d8c120332c0bf916b5b576aff8374af1ed "kernel: Add headerTip method to notifications"
- [ ] 1be1c16b70edc5def98eb1903cee08b5d546ba88 "kernel: Add progress method to notifications"
- [ ] 483fe64956179872e5821483474c52d955949e40 "kernel: Add warning method to notifications"
- [ ] 3785d38417618e921c5233cf14aa3c802f37387b "refactor: Move ScheduleBatchPriority to its own file"
- [
...