💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1724864056)
added

💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1724778335)
Seems fine to do, but won't add it to this PR because I want to limit its scope. Agree just the last paragraph is sufficient.

💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1724882313)
added

💬 hebasto commented on pull request "guix: (explicitly) build Linux GCC with `--enable-cet`":
(https://github.com/bitcoin/bitcoin/pull/30438#issuecomment-2301941489)
> I don't think we need to do anything here

In the current state, the statement from the PR description:
> CET is enabled on Linux/x86

is not accurate.

💬 hebasto commented on pull request "depends: Fix CMake-generated `libzmq.pc` file":
(https://github.com/bitcoin/bitcoin/pull/30508#issuecomment-2301960884)
> > I'd rather wait until upstream weighs in though, since we don't need this at least until we switch to CMake.
>
> Looks like upstream either forgot about, or missed this, so I guess someone should followup.

It has been just [merged](https://github.com/zeromq/libzmq/commit/5f408ba371ae4789549fb4696dcccd2ac946b7eb).

💬 dergoegge commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#issuecomment-2301983925)
The naming of the files and tests is a bit inconsistent, I would suggest the following:

* `txdownloadman.h`, `txdownloadman.cpp`, `txdownloadman_impl.h` and `txdownloadman_impl.cpp` with the classes: `TxDownloadManager` and `TxDownloadManagerImpl`
* `fuzz/txdownloadman.cpp`: `txdownloadman` + `txdownloadman_impl`
* `fuzz/txdownloadman_one_honest_peer.cpp`: `txdownloadman_one_honest_peer`

I don't care about the specific names just the consistency, e.g. don't mix `tx_download` and `txdownl
...

💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1725039042)
think you added the other way, that if `should_validate` is false, it must have a package to validate, which is incorrect(and caught by our 1p1c tests)

💬 brunoerg commented on pull request "fuzz: speed up addrman":
(https://github.com/bitcoin/bitcoin/pull/30688#discussion_r1725039993)
Sounds good, I'll address it.

💬 brunoerg commented on pull request "fuzz: speed up addrman":
(https://github.com/bitcoin/bitcoin/pull/30688#discussion_r1725041385)
done

💬 brunoerg commented on pull request "fuzz: speed up addrman":
(https://github.com/bitcoin/bitcoin/pull/30688#issuecomment-2302041809)
Force-pushed addressing https://github.com/bitcoin/bitcoin/pull/30688#discussion_r1724897721

💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1725043966)
Even though the prior version had many ACKs, this reaches another level of awesome.
Initial version using `""_hex` now pushed in daba1a25a62e72e9797a134c6377d17a9274a25f with @l0rinc as main author of that commit! :tada:

Resolving this specific thread.

💬 ryanofsky commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1725063869)
Makes sense that const can expose things even in simple cases and lead to insights and improvements like the one you are suggesting. In this case, I think "if starts with prefix call remove_prefix" might be more straightforward than "if starts with prefix call substr and return portion of the string after the prefix" but the difference is not great and I take your point in general const can have benefits in places like this.

💬 furszy commented on issue "bitcoind shouldn't be shutdown automatically despite wallet synchronisation error":
(https://github.com/bitcoin/bitcoin/issues/30671#issuecomment-2302084858)
> As a user, I want to start bitcoind irrespectively of coins being in a wallet.

There are four ways to opt-out of running a wallet:
1) Do not run any wallet: `-nowallet=1`.
2) Do not run a specific wallet `-disablewallet=<wallet_name>`.
3) Remove the wallet json object manually from the settings.json file.
4) Disable wallet support at the binaries level during build.

Any of them will work fine in your specific case of not caring about your existing wallet.

💬 pablomartin4btc commented on pull request "validation: assumeutxo params mainnet":
(https://github.com/bitcoin/bitcoin/pull/28553#issuecomment-2302232259)
<details>
<summary>Continued testing on a full node until validation is finished (<code>getchainstate</code>) and fully synced, also on a pruned node (<code>-prune=30000</code>).</summary>

```
./src/bitcoin-cli -datadir=${AU_DATADIR} getchainstates
{
"headers": 857301,
"chainstates": [
{
"blocks": 857301,
"bestblockhash": "000000000000000000017a865975aac40cdb7d05f4010a6e915ffe7af260a151",
"difficulty": 86871474313761.95,
"verificationprogress": 0.9999
...

🤔 pablomartin4btc reviewed a pull request: "validation: assumeutxo params mainnet"
(https://github.com/bitcoin/bitcoin/pull/28553#pullrequestreview-2251032570)
<details>
<summary>Continued testing on a full node until validation is finished (<code>getchainstate</code>) and fully synced, also on a pruned node (<code>-prune=30000</code>).</summary>

```
./src/bitcoin-cli -datadir=${AU_DATADIR} getchainstates
{
"headers": 857301,
"chainstates": [
{
"blocks": 857301,
"bestblockhash": "000000000000000000017a865975aac40cdb7d05f4010a6e915ffe7af260a151",
"difficulty": 86871474313761.95,
"verificationprogress": 0.9999
...

👍 ryanofsky approved a pull request: "wallet: Write best block to disk before backup"
(https://github.com/bitcoin/bitcoin/pull/30678#pullrequestreview-2251033417)
Code review ACK 888a167446901c12960e6776613a3a1fb9789f59. I think this would be a good change and improvement, however:

- I don't have a high level of confidence about this so would be interested in opinions from achow, furszy, and others who could point out other effects and drawbacks.
- If we are going to update the locator when backing up wallets, it seems like it might make sense to update it in other cases like when unloading wallets (https://github.com/bitcoin/bitcoin/pull/30678#discus
...

💬 ryanofsky commented on issue "bitcoind shouldn't be shutdown automatically despite wallet synchronisation error":
(https://github.com/bitcoin/bitcoin/issues/30671#issuecomment-2302254740)
Agree with furszy and marco that current behavior makes sense and there are a lot of existing options for dealing with this situation. I could imagine adding new options, however, like -reindex=auto to reindex automatically when needed, or `-allowfail=wallets` to ignore failures from wallets `-allowfail=indexes` to allow starting up when wallets or indexes fail to load.

👋 hebasto's pull request is ready for review: "build: Mark `x86_64-linux-gnu` release binaries as CET-enabled"
(https://github.com/bitcoin/bitcoin/pull/30685)

👍 ryanofsky approved a pull request: "fix: handle invalid `-rpcbind` port earlier"
(https://github.com/bitcoin/bitcoin/pull/30679#pullrequestreview-2251103866)
Code review ACK 8892150c2d8ea9e09046e288cecd4c35472c0267. Looks good! Nice to have earlier feedback and clearer checking for these options. I left a suggestion, but it's not important, so feel free to ignore

💬 ryanofsky commented on pull request "fix: handle invalid `-rpcbind` port earlier":
(https://github.com/bitcoin/bitcoin/pull/30679#discussion_r1725239115)
In commit "fix: handle invalid rpcbind port earlier" (8892150c2d8ea9e09046e288cecd4c35472c0267)

Not important, but to make AppInitMain function shorter and make the bugfix commit fix more readable you could consider adding an earlier refactoring commit the moves these for loops into a `CheckHostPortOptions` function, and have AppinitMain call it like `if (!CheckHostPortOptions(args)) return false;`