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

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

💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1724840174)
agree, though leaving out of this PR to limit its scope. Could add a nonsegwit parent + segwit child test to be added to p2p_orphan_handling.py. Maybe a good first issue.

💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1724832923)
Renamed it to `first_time_failure` in a preceding commit.

Yeah that's correct. Basically, if this is the first time a tx is being rejected from mempool, we consider putting it in `vExtraTxnForCompact`. If it's not the first time, we don't need to do it again, presumably because we don't want duplicates (it's a ring buffer).

Same thing for adding something as an orphan. If this bool is false, it means the tx is already an orphan or it's the low feerate parent in a package. Either way we don
...

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