💬 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.

💬 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.

💬 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.

💬 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/
...

💬 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
```

💬 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

💬 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.

💬 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.

✅ fanquake closed a pull request: "Check for the awk script before executing it"
(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.

💬 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.

💬 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?

📝 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
...

💬 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)
...

✳️ fanquake pushed commits to a branch: bitcoin/bitcoin:master
(https://github.com/bitcoin/bitcoin/compare/2c1fe27bf3c1...d6ef44cccbdf)
Merge bitcoin/bitcoin#27081: Modernize rpcauth.py

e4e17907b686c25dda91e569645a8f501ca48f90 Modernize rpcauth.py and its tests (Pieter Wuille)

Pull request description:

Use Python3 constructions, and f-strings.

ACKs for top commit:
jamesob:
Github ACK https://github.com/bitcoin/bitcoin/pull/27081/commits/e4e17907b686c25dda91e569645a8f501ca48f90

Tree-SHA512: 005573d967e04400fec727f45739f138879be703e692745c0a639272d37d221d230f388de23f2615cb954bb47179fb46e53da0410ae9f0865319b91bb2dc01f4

🚀 fanquake merged a pull request: "Modernize rpcauth.py"
(https://github.com/bitcoin/bitcoin/pull/27081)

💬 hebasto commented on pull request "guix: consolidate to glibc 2.27 for Linux builds":
(https://github.com/bitcoin/bitcoin/pull/27029#issuecomment-1429668279)
Guix builds:
```
851f4116c395a6b89475871a9763a111abad3951424276135931d80b1c09615e guix-build-d5d4b75840b4/output/aarch64-linux-gnu/SHA256SUMS.part
bb9d3867b8f7dae5f9f8c326aa1b0e9fa28964659b3c7187e743152044c9dbe8 guix-build-d5d4b75840b4/output/aarch64-linux-gnu/bitcoin-d5d4b75840b4-aarch64-linux-gnu-debug.tar.gz
e4c0ab4883993362aea104ea776102a5d5beaa39a22e9aa22fd27a81d36415dc guix-build-d5d4b75840b4/output/aarch64-linux-gnu/bitcoin-d5d4b75840b4-aarch64-linux-gnu.tar.gz
01b239015610aea0c47
...

💬 Ayms commented on issue "Allow several OP_RETURN in one tx and no limited size":
(https://github.com/bitcoin/bitcoin/issues/27043#issuecomment-1429678640)
@MarcoFalke super simple...

@Semisol as mentionned in the thread there are so many ways to store things that there is no rationale to impose a limit in OP_RETURN, this is cleary a handicap for bitcoin and future uses/evolutions, bitcoin can clearly do far better what ethereum and all the messy stuff around pretend to do but we need this change

💬 furszy commented on pull request "refactor: wallet, remove global 'ArgsManager' dependency":
(https://github.com/bitcoin/bitcoin/pull/26889#discussion_r1105772605)
np, small miscommunication. Done now, all of them tackled now. Thanks!

💬 Sjors commented on pull request "p2p, validation: Don't download witnesses for assumed-valid blocks when running in prune mode":
(https://github.com/bitcoin/bitcoin/pull/27050#issuecomment-1429695634)
Making witness skipping opt-in makes sense. In addition our GUI initial setup wizard could help the user with this (as it does with suggesting pruning).

💬 furszy commented on pull request "refactor: wallet, remove global 'ArgsManager' dependency":
(https://github.com/bitcoin/bitcoin/pull/26889#issuecomment-1429700293)
Feedback tackled.
Moved remaining variables to signed type. Small [diff](https://github.com/bitcoin/bitcoin/compare/fafaa7a7e15a49da0214ce9546a42117c63e611f..d90b54b4aa18d60faa0075348fbb29a513558098).