💬 laanwj commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1567771033)
`header.type` is compared against `BTREE_LEAF` and `BTREE_INTERNAL`, which are the same cases as handled in the `switch()` statement below, which also errors in the default page (though with a slightly different error), so this check is redundant?
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1567771033)
`header.type` is compared against `BTREE_LEAF` and `BTREE_INTERNAL`, which are the same cases as handled in the `switch()` statement below, which also errors in the default page (though with a slightly different error), so this check is redundant?
💬 achow101 commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r1567767610)
In 000000000fbad5312fe6975959b0ab9ca5daeb16 "Add Base58 Address Prefix Decoder"
It seems a bit odd to me to do all of this work to compute these prefixes just for error messaging. ISTM it would be a lot simpler to just hard code them into chainparams anyways. It's not as if the prefixes will ever change, and new ones are unlikely to be added.
Merging this code seems like an additional maintenance burden for very little gain.
Also, if you insist on keeping this code, it should be placed
...
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r1567767610)
In 000000000fbad5312fe6975959b0ab9ca5daeb16 "Add Base58 Address Prefix Decoder"
It seems a bit odd to me to do all of this work to compute these prefixes just for error messaging. ISTM it would be a lot simpler to just hard code them into chainparams anyways. It's not as if the prefixes will ever change, and new ones are unlikely to be added.
Merging this code seems like an additional maintenance burden for very little gain.
Also, if you insist on keeping this code, it should be placed
...
💬 pinheadmz commented on pull request "ZMQ: Support UNIX domain sockets":
(https://github.com/bitcoin/bitcoin/pull/27679#issuecomment-2059679632)
Thanks so much for your review @laanwj addressed your comments
(https://github.com/bitcoin/bitcoin/pull/27679#issuecomment-2059679632)
Thanks so much for your review @laanwj addressed your comments
💬 pinheadmz commented on pull request "system: use %LOCALAPPDATA% as default datadir on windows":
(https://github.com/bitcoin/bitcoin/pull/27064#issuecomment-2059694500)
> Could rebase for fresh CI, if still relevant?
rebased on master
(https://github.com/bitcoin/bitcoin/pull/27064#issuecomment-2059694500)
> Could rebase for fresh CI, if still relevant?
rebased on master
💬 ryanofsky commented on pull request "index: race fix, lock cs_main while 'm_synced' is subject to change":
(https://github.com/bitcoin/bitcoin/pull/29867#issuecomment-2059695927)
> Though maybe if the timing is really unlucky you'd get lots of Commit() calls?
That's a good point. If Commit() is slow and it has to be called more than once, it could make index sync slower, even if it blocks validation code less.
There's a choice about what to do when the Commit() call at the end of the sync is slow and a new block is connected while it is executing.
1. Currently, the new block may get ignored and the index could be corrupted.
2. Before 0faafb57f8298547949cbc0044e
...
(https://github.com/bitcoin/bitcoin/pull/29867#issuecomment-2059695927)
> Though maybe if the timing is really unlucky you'd get lots of Commit() calls?
That's a good point. If Commit() is slow and it has to be called more than once, it could make index sync slower, even if it blocks validation code less.
There's a choice about what to do when the Commit() call at the end of the sync is slow and a new block is connected while it is executing.
1. Currently, the new block may get ignored and the index could be corrupted.
2. Before 0faafb57f8298547949cbc0044e
...
💬 pinheadmz commented on pull request "blockstorage: do not flush block to disk if it is already there":
(https://github.com/bitcoin/bitcoin/pull/27039#discussion_r1567788078)
I'm not sure I understand. The previous code expected a crash so the test would know exactly when to check for the expected log message. In the new code bitcoind just runs fine so we have to poll the log for the success message.
(https://github.com/bitcoin/bitcoin/pull/27039#discussion_r1567788078)
I'm not sure I understand. The previous code expected a crash so the test would know exactly when to check for the expected log message. In the new code bitcoind just runs fine so we have to poll the log for the success message.
💬 laanwj commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1567792369)
Note that the argument to `ignore()` is unsigned (a `size_t`), might want to error out here if somehow `index < pos`.
(same for the occurence in `InternalPage`)
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1567792369)
Note that the argument to `ignore()` is unsigned (a `size_t`), might want to error out here if somehow `index < pos`.
(same for the occurence in `InternalPage`)
💬 achow101 commented on pull request "Wallet: Add GetFullBalance(...) which included used balance":
(https://github.com/bitcoin/bitcoin/pull/29062#discussion_r1567793472)
This doesn't need to be a separate function, just have `GetBalance` include `m_mine_used`. It's up to the caller to figure out what to do with that information.
(https://github.com/bitcoin/bitcoin/pull/29062#discussion_r1567793472)
This doesn't need to be a separate function, just have `GetBalance` include `m_mine_used`. It's up to the caller to figure out what to do with that information.
💬 laanwj commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1567799862)
It looks like `fclose()` does nothing if `db_file.IsNull()` already holds.
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1567799862)
It looks like `fclose()` does nothing if `db_file.IsNull()` already holds.
💬 achow101 commented on pull request "tests: improve wallet multisig descriptor test and docs":
(https://github.com/bitcoin/bitcoin/pull/29154#discussion_r1567805211)
In 8a2021c0f4f40f6f8a37dd5619ecfa3f38084679 "tests: assert xpub order in sortedmulti descriptors do not matter"
It's not clear to me what this `random.sample` is supposed to do or how it improves the test.
(https://github.com/bitcoin/bitcoin/pull/29154#discussion_r1567805211)
In 8a2021c0f4f40f6f8a37dd5619ecfa3f38084679 "tests: assert xpub order in sortedmulti descriptors do not matter"
It's not clear to me what this `random.sample` is supposed to do or how it improves the test.
💬 achow101 commented on pull request "tests: improve wallet multisig descriptor test and docs":
(https://github.com/bitcoin/bitcoin/pull/29154#discussion_r1567795960)
In 80ad8690e25c1ecab2ab1ff282882d5a6745f496 "tests: improve wallet multisig descriptor test and docs"
I don't think it's necessary to explain where the descriptor came from, just a description of it is fine.
(https://github.com/bitcoin/bitcoin/pull/29154#discussion_r1567795960)
In 80ad8690e25c1ecab2ab1ff282882d5a6745f496 "tests: improve wallet multisig descriptor test and docs"
I don't think it's necessary to explain where the descriptor came from, just a description of it is fine.
💬 maflcko commented on issue "Intermittent issue in test/ipc_tests.cpp Fatal glibc error: pthread_mutex_lock.c:450 (__pthread_mutex_lock_full): assertion failed: e != ESRCH || !robust":
(https://github.com/bitcoin/bitcoin/issues/29889#issuecomment-2059743863)
Actually, I think the logs are complete for the log level `test_suite`. If more details are needed `test_suite` could be changed to `all`.
(https://github.com/bitcoin/bitcoin/issues/29889#issuecomment-2059743863)
Actually, I think the logs are complete for the log level `test_suite`. If more details are needed `test_suite` could be changed to `all`.
💬 maflcko commented on pull request "blockstorage: do not flush block to disk if it is already there":
(https://github.com/bitcoin/bitcoin/pull/27039#discussion_r1567825773)
`wait_for_debug_log` (the variant of the function that accepts raw bytes) does not have a `sleep`, so it will try to maxx out IO at 100%, no?
(https://github.com/bitcoin/bitcoin/pull/27039#discussion_r1567825773)
`wait_for_debug_log` (the variant of the function that accepts raw bytes) does not have a `sleep`, so it will try to maxx out IO at 100%, no?
💬 achow101 commented on pull request "test: Handle functional test disk-full error":
(https://github.com/bitcoin/bitcoin/pull/29335#discussion_r1567828103)
In 858fa78637041ae704005d4b6e564dd8245f4b5d "test: Handle functional test disk-full error"
The `+`s at the beginning of the lines are unecessary.
```suggestion
f"Test execution data left in {tmpdir}.\n"
f"Additional storage is needed to execute testing.")
```
(https://github.com/bitcoin/bitcoin/pull/29335#discussion_r1567828103)
In 858fa78637041ae704005d4b6e564dd8245f4b5d "test: Handle functional test disk-full error"
The `+`s at the beginning of the lines are unecessary.
```suggestion
f"Test execution data left in {tmpdir}.\n"
f"Additional storage is needed to execute testing.")
```
💬 laanwj commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1567831243)
Could add a check that `is_key` is true at the end. If not, there's an odd number of values, which would be odd?
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1567831243)
Could add a check that `is_key` is true at the end. If not, there's an odd number of values, which would be odd?
💬 russeree commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r1567835969)
https://github.com/bitcoin/bitcoin/pull/27260#discussion_r1239035489
The choice to do this was a response to the above comment. I would agree chainparams would be the place to implement a hardcoded (char[], stringview) prefix. Another advantage is that lookup is likely faster; so if a user is frequently getting errors it may save a few them a few cycles.
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r1567835969)
https://github.com/bitcoin/bitcoin/pull/27260#discussion_r1239035489
The choice to do this was a response to the above comment. I would agree chainparams would be the place to implement a hardcoded (char[], stringview) prefix. Another advantage is that lookup is likely faster; so if a user is frequently getting errors it may save a few them a few cycles.
💬 maflcko commented on pull request "build: add `-Wundef`":
(https://github.com/bitcoin/bitcoin/pull/29876#discussion_r1567840431)
> say, when you forget to include bitcoin-config)
In theory, there is a linter to check this. However, the linter could be tricked by guarding the include by `#if defined(HAVE_C0NFIG_H)` (`O` replaced by `0`). This is fixed in #29494.
(https://github.com/bitcoin/bitcoin/pull/29876#discussion_r1567840431)
> say, when you forget to include bitcoin-config)
In theory, there is a linter to check this. However, the linter could be tricked by guarding the include by `#if defined(HAVE_C0NFIG_H)` (`O` replaced by `0`). This is fixed in #29494.
💬 Krokochik commented on issue "importdescriptors hanging on importing/updating descriptor with large range":
(https://github.com/bitcoin/bitcoin/issues/25895#issuecomment-2059776680)
@willcl-ark no it doesn't
(https://github.com/bitcoin/bitcoin/issues/25895#issuecomment-2059776680)
@willcl-ark no it doesn't
💬 laanwj commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1567845152)
Instead of casting back and forth and having a `GetRealType()` it would, imo, be more straightforward to split the delete flag off from the type in `Unserialize` before casting to `RecordType`. So to have
```
RecordType type;
bool deleted;
```
Then these accessors could go.
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1567845152)
Instead of casting back and forth and having a `GetRealType()` it would, imo, be more straightforward to split the delete flag off from the type in `Unserialize` before casting to `RecordType`. So to have
```
RecordType type;
bool deleted;
```
Then these accessors could go.
🤔 achow101 reviewed a pull request: "tests: add functional test for miniscript decaying multisig"
(https://github.com/bitcoin/bitcoin/pull/29156#pullrequestreview-2004420996)
e0b0a385350d8f7a9d00542cc3d14bb47224d889 "tests: miniscript decaying multisig signing order does not matter" could be squashed into de93b863583921929bc16feb7a0bf5decfbc7ac5 "tests: add functional test for miniscript decaying multisig".
(https://github.com/bitcoin/bitcoin/pull/29156#pullrequestreview-2004420996)
e0b0a385350d8f7a9d00542cc3d14bb47224d889 "tests: miniscript decaying multisig signing order does not matter" could be squashed into de93b863583921929bc16feb7a0bf5decfbc7ac5 "tests: add functional test for miniscript decaying multisig".