💬 martinus commented on pull request "Add simulation-based `CCoinsViewCache` fuzzer":
(https://github.com/bitcoin/bitcoin/pull/27011#issuecomment-1429201435)
@sipa While rebasing #25325 I saw that the fuzzer in `coins_view.cpp` still uses `CCoinsMap coins_map;` and doesn't use the deterministic seeds, shouldn't this fuzzer do that as well?
I can add it while rebasing, I'm touching that line anyways. Unless you want to do this separately
(https://github.com/bitcoin/bitcoin/pull/27011#issuecomment-1429201435)
@sipa While rebasing #25325 I saw that the fuzzer in `coins_view.cpp` still uses `CCoinsMap coins_map;` and doesn't use the deterministic seeds, shouldn't this fuzzer do that as well?
I can add it while rebasing, I'm touching that line anyways. Unless you want to do this separately
💬 hebasto commented on pull request "Check for the awk script before executing it":
(https://github.com/bitcoin/bitcoin/pull/27095#issuecomment-1429249881)
@Harshil-Jani
Thank you for your contribution! I think it is more appropriate to submit your patch to the [upstream project](https://github.com/kadwanev/retry).
(https://github.com/bitcoin/bitcoin/pull/27095#issuecomment-1429249881)
@Harshil-Jani
Thank you for your contribution! I think it is more appropriate to submit your patch to the [upstream project](https://github.com/kadwanev/retry).
💬 martinus commented on pull request "Add pool based memory resource":
(https://github.com/bitcoin/bitcoin/pull/25325#issuecomment-1429293948)
Rebased due to #27011. Note that this adds a `/*deterministic=*/true` [here in test/fuzz/coins_view.cpp](https://github.com/bitcoin/bitcoin/pull/25325/commits/78f597be2879c39d9d2b98e21ed0120d2308de20#diff-1ef3b6a1936b50f3d5ec4a1786d9e2d63d1a3e1815b103e67f20601995f355b4R119), see `git range-diff 0007d69...78f597b`
(https://github.com/bitcoin/bitcoin/pull/25325#issuecomment-1429293948)
Rebased due to #27011. Note that this adds a `/*deterministic=*/true` [here in test/fuzz/coins_view.cpp](https://github.com/bitcoin/bitcoin/pull/25325/commits/78f597be2879c39d9d2b98e21ed0120d2308de20#diff-1ef3b6a1936b50f3d5ec4a1786d9e2d63d1a3e1815b103e67f20601995f355b4R119), see `git range-diff 0007d69...78f597b`
💬 TheCharlatan commented on pull request "refactor, kernel: Remove gArgs accesses from dbwrapper and txdb":
(https://github.com/bitcoin/bitcoin/pull/25862#issuecomment-1429298769)
Code review ACK aadd7c5b9b43a38beaa954b4cb8c2fff55f2200f
(https://github.com/bitcoin/bitcoin/pull/25862#issuecomment-1429298769)
Code review ACK aadd7c5b9b43a38beaa954b4cb8c2fff55f2200f
👍 TheCharlatan approved a pull request: "refactor: wallet, remove global 'ArgsManager' dependency"
(https://github.com/bitcoin/bitcoin/pull/26889)
(https://github.com/bitcoin/bitcoin/pull/26889)
💬 MarcoFalke commented on pull request "refactor: Move coin_control variable to test setup section":
(https://github.com/bitcoin/bitcoin/pull/26154#issuecomment-1429330974)
Closing for now due to controversy.
(https://github.com/bitcoin/bitcoin/pull/26154#issuecomment-1429330974)
Closing for now due to controversy.
✅ MarcoFalke closed a pull request: "refactor: Move coin_control variable to test setup section"
(https://github.com/bitcoin/bitcoin/pull/26154)
(https://github.com/bitcoin/bitcoin/pull/26154)
💬 MarcoFalke commented on pull request "Use designated initializers":
(https://github.com/bitcoin/bitcoin/pull/24531#discussion_r1105484093)
Just for clarity and documentation. This code is wrong as-is and has long been removed. Omitting the field name here can lead to bugs, see https://godbolt.org/z/9TnoMKMT3 for a playground.
(https://github.com/bitcoin/bitcoin/pull/24531#discussion_r1105484093)
Just for clarity and documentation. This code is wrong as-is and has long been removed. Omitting the field name here can lead to bugs, see https://godbolt.org/z/9TnoMKMT3 for a playground.
💬 hebasto commented on pull request "refactor: wallet, remove global 'ArgsManager' dependency":
(https://github.com/bitcoin/bitcoin/pull/26889#discussion_r1105535083)
Sorry for not being explicit in my previous comments. There are more cases where `uint64_t` could be replaced with `int64_t`:
```suggestion
int64_t m_keypool_size GUARDED_BY(cs_desc_man){DEFAULT_KEYPOOL_SIZE};
```
and parameter `keypool_size` in (member) functions.
(https://github.com/bitcoin/bitcoin/pull/26889#discussion_r1105535083)
Sorry for not being explicit in my previous comments. There are more cases where `uint64_t` could be replaced with `int64_t`:
```suggestion
int64_t m_keypool_size GUARDED_BY(cs_desc_man){DEFAULT_KEYPOOL_SIZE};
```
and parameter `keypool_size` in (member) functions.
💬 hebasto commented on pull request "build: Check usages of #if defined(...)":
(https://github.com/bitcoin/bitcoin/pull/25302#issuecomment-1429426979)
@brokenprogrammer
> I've squashed and updated the commit message according to the contributing guidelines.
A https://github.com/bitcoin/bitcoin/pull/25302#discussion_r1104616746 still needs to be addressed.
(https://github.com/bitcoin/bitcoin/pull/25302#issuecomment-1429426979)
@brokenprogrammer
> I've squashed and updated the commit message according to the contributing guidelines.
A https://github.com/bitcoin/bitcoin/pull/25302#discussion_r1104616746 still needs to be addressed.
💬 willcl-ark commented on pull request "Convert ArgsManager::GetDataDir to a read-only function":
(https://github.com/bitcoin/bitcoin/pull/27073#issuecomment-1429434184)
> I will check this out, specifically w.r.t this comment which I had not been considering:
I don't believe this fully closes the loop on #16220, but I am also not sure that we ever will close that fully whilst also providing backwards-compatibility...
There can exist currently two different working wallet configurations:
1. Legacy: wallets in subfolders in top-level, e.g. `~/.bitcoin/test-wallet/wallet.dat` or `~/.bitcoin/regtest/test-wallet/wallet.dat`
2. Current: wallets in wallets/
...
(https://github.com/bitcoin/bitcoin/pull/27073#issuecomment-1429434184)
> I will check this out, specifically w.r.t this comment which I had not been considering:
I don't believe this fully closes the loop on #16220, but I am also not sure that we ever will close that fully whilst also providing backwards-compatibility...
There can exist currently two different working wallet configurations:
1. Legacy: wallets in subfolders in top-level, e.g. `~/.bitcoin/test-wallet/wallet.dat` or `~/.bitcoin/regtest/test-wallet/wallet.dat`
2. Current: wallets in wallets/
...
💬 TheCharlatan commented on pull request "build: use _FORTIFY_SOURCE=3":
(https://github.com/bitcoin/bitcoin/pull/27027#issuecomment-1429435999)
Concept ACK
Ran this on Ubuntu 22.04 with glibc 2.35 and clang 14. Also did not observe a significant slowdown of the benchmarks.
On master:
```
objdump -d src/bitcoind | grep "_chk@plt" | wc -l
...
36
```
This branch:
```
objdump -d src/bitcoind | grep "_chk@plt" | wc -l
...
76
```
(https://github.com/bitcoin/bitcoin/pull/27027#issuecomment-1429435999)
Concept ACK
Ran this on Ubuntu 22.04 with glibc 2.35 and clang 14. Also did not observe a significant slowdown of the benchmarks.
On master:
```
objdump -d src/bitcoind | grep "_chk@plt" | wc -l
...
36
```
This branch:
```
objdump -d src/bitcoind | grep "_chk@plt" | wc -l
...
76
```
💬 willcl-ark commented on pull request "Convert ArgsManager::GetDataDir to a read-only function":
(https://github.com/bitcoin/bitcoin/pull/27073#discussion_r1105549095)
Thanks, I have updated to `EnsureDataDirNet` as suggested
(https://github.com/bitcoin/bitcoin/pull/27073#discussion_r1105549095)
Thanks, I have updated to `EnsureDataDirNet` as suggested
💬 willcl-ark commented on pull request "Convert ArgsManager::GetDataDir to a read-only function":
(https://github.com/bitcoin/bitcoin/pull/27073#discussion_r1105552243)
Now updated to `EnsureDataDirNet` which always creates the `wallets/` path if it's creating a new DataDir, but otherwise doesn't.
(https://github.com/bitcoin/bitcoin/pull/27073#discussion_r1105552243)
Now updated to `EnsureDataDirNet` which always creates the `wallets/` path if it's creating a new DataDir, but otherwise doesn't.
💬 fanquake commented on pull request "Check for the awk script before executing it":
(https://github.com/bitcoin/bitcoin/pull/27095#issuecomment-1429472868)
Yep. Feel free to send this change upstream.
(https://github.com/bitcoin/bitcoin/pull/27095#issuecomment-1429472868)
Yep. Feel free to send this change upstream.
✅ fanquake closed a pull request: "Check for the awk script before executing it"
(https://github.com/bitcoin/bitcoin/pull/27095)
(https://github.com/bitcoin/bitcoin/pull/27095)
💬 fanquake commented on pull request "Wallet: Zero out wallet master key upon locking so it doesn't persist in memory":
(https://github.com/bitcoin/bitcoin/pull/27080#issuecomment-1429473503)
Added this to #26878 for backporting to 24.x.
(https://github.com/bitcoin/bitcoin/pull/27080#issuecomment-1429473503)
Added this to #26878 for backporting to 24.x.
💬 MarcoFalke commented on pull request "verify-commits: Bump trusted git root to after most recent laanwj merge":
(https://github.com/bitcoin/bitcoin/pull/27076#issuecomment-1429480424)
lgtm, Approach ACK
I haven't tested this, because I use a trusted git root set by myself anyway, but I can't see a reason not to do this. This was also done last time in commit d4b3dc5b0a726cc4cc7a8467be43126e78f841cf, so it makes sense to do the same approach again.
(https://github.com/bitcoin/bitcoin/pull/27076#issuecomment-1429480424)
lgtm, Approach ACK
I haven't tested this, because I use a trusted git root set by myself anyway, but I can't see a reason not to do this. This was also done last time in commit d4b3dc5b0a726cc4cc7a8467be43126e78f841cf, so it makes sense to do the same approach again.
💬 willcl-ark commented on pull request "Convert ArgsManager::GetDataDir to a read-only function":
(https://github.com/bitcoin/bitcoin/pull/27073#issuecomment-1429498959)
I don't particularly want to feature-creep too much, but another adjacent issue is https://github.com/bitcoin/bitcoin/issues/19990
Which could potentially be addressed in this PR too if desired?
(https://github.com/bitcoin/bitcoin/pull/27073#issuecomment-1429498959)
I don't particularly want to feature-creep too much, but another adjacent issue is https://github.com/bitcoin/bitcoin/issues/19990
Which could potentially be addressed in this PR too if desired?
📝 theStack opened a pull request: "script: remove unused bitwise `CScriptNum` operators"
(https://github.com/bitcoin/bitcoin/pull/27096)
These overloaded operators were introduced almost seven years ago in the implementation of OP_CHECKSEQUENCEVERIFY (BIP112, PR #7524, commit 53e53a33c939949665f60d5eeb82abbb21f97128), have never used since outside of fuzzing. One could argue to keep those around solely for consistency reasons, but I think its preferable to get rid of methods that are likely never used. Even the so-far only use of the & operator (checking the OP_CSV argument for the SEQUENCE_LOCKTIME_DISABLE_FLAG), could be remove
...
(https://github.com/bitcoin/bitcoin/pull/27096)
These overloaded operators were introduced almost seven years ago in the implementation of OP_CHECKSEQUENCEVERIFY (BIP112, PR #7524, commit 53e53a33c939949665f60d5eeb82abbb21f97128), have never used since outside of fuzzing. One could argue to keep those around solely for consistency reasons, but I think its preferable to get rid of methods that are likely never used. Even the so-far only use of the & operator (checking the OP_CSV argument for the SEQUENCE_LOCKTIME_DISABLE_FLAG), could be remove
...
💬 stickies-v commented on pull request "wallet: SecureString to allow null characters":
(https://github.com/bitcoin/bitcoin/pull/27068#issuecomment-1429565498)
> I would be surprised if any password manager produced a passphrase that included non-typeable characters.
I googled "random password generator" and with the [second result](https://passwordsgenerator.net/) I was able to generate a null-character containing password (\039"`\9%|+']1+5) in less than a minute (disabled using letters to speed it up, but otherwise using default settings). It seems like indeed a fair amount of generators do not use backslashes as symbols (e.g. Bitwarden, Lastpass)
...
(https://github.com/bitcoin/bitcoin/pull/27068#issuecomment-1429565498)
> I would be surprised if any password manager produced a passphrase that included non-typeable characters.
I googled "random password generator" and with the [second result](https://passwordsgenerator.net/) I was able to generate a null-character containing password (\039"`\9%|+']1+5) in less than a minute (disabled using letters to speed it up, but otherwise using default settings). It seems like indeed a fair amount of generators do not use backslashes as symbols (e.g. Bitwarden, Lastpass)
...