💬 sipa commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1399160874)
To check `a/b > c/d` you can instead check `a*d > c*b`, which avoids divisions (which are an order of magnitude slower than multiplication).

💬 sipa commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1399161977)
I believe that's an idea we've toyed with (calling it "sibling eviction"), but so far it isn't clear how to align that with DoS prevention.

💬 hebasto commented on pull request "[26.x] Final Changes":
(https://github.com/bitcoin/bitcoin/pull/28872#discussion_r1399162674)
I'm sorry for that late comment. Mind adding the following stuff:
```
- The transaction list in the GUI to no longer provide a special category for “payment to yourself”. Now transactions that have both inputs and outputs that affect the wallet are displayed on separate lines for spending and receiving. (gui#119)

- A new menu option allows migrating a legacy wallet based on keys and implied output script types stored in BerkeleyDB (BDB) to a modern wallet that uses descriptors stored in SQL
...

💬 ismaelsadeeq commented on pull request "Fee Estimator updates from Validation Interface/CScheduler thread":
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1399162733)
Fixed

💬 ismaelsadeeq commented on pull request "Fee Estimator updates from Validation Interface/CScheduler thread":
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1399163164)
I updated the commit message 👍🏾

💬 ismaelsadeeq commented on pull request "Fee Estimator updates from Validation Interface/CScheduler thread":
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1399163551)
Updated with your suggestion

💬 ismaelsadeeq commented on pull request "Fee Estimator updates from Validation Interface/CScheduler thread":
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1399165583)
This will change the input of the `CBlockPolicyEstimator` fuzzing test, I added a docstring `/*inBlock*/`.

⚠️ dergoegge opened an issue: "fuzz, coinselection: Assertion 'result_bnb->GetChange(coin_params.m_cost_of_change, CAmount{0}) == 0' failed"
(https://github.com/bitcoin/bitcoin/issues/28918)
```
$ echo "PACVlVuVlZWVlZUE3pUAANNRAFEA09NRUb9RUVFR/wD/AP//AP8AWwD//wcErq6urv///////wD/4wAAAAD4a9cA////ra2tra2tra2tra2VlZWVMGA5OTk5OZWVlZWVlZWVlZWVlZWVlYUVJwyq6wEAlZWblZWVlZWVlZWVlZWVlZWblZWVlZWVlZWVlZWV//+V/5WV/////z4AAAAALAtfAgAAX/8=" | base64 --decode > coinselection-d97eed2ff63da56af72c8c858c560a7c6f2aef45.crash
$ FUZZ=coinselection ./src/test/fuzz/fuzz coinselection-d97eed2ff63da56af72c8c858c560a7c6f2aef45.crash
INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 189972
...

💬 dergoegge commented on issue "fuzz, coinselection: Assertion 'result_bnb->GetChange(coin_params.m_cost_of_change, CAmount{0}) == 0' failed":
(https://github.com/bitcoin/bitcoin/issues/28918#issuecomment-1819018704)
@murchandamus @furszy @brunoerg what's the status on this?

💬 ismaelsadeeq commented on pull request "Fee Estimator updates from Validation Interface/CScheduler thread":
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1399168585)
To override virtual functions from the `CValidationInterface` class, they must have same number of parameters and types to the corresponding virtual function in `CValidationInterface.

💬 maflcko commented on pull request "refactor: P2P transport without serialize version and type":
(https://github.com/bitcoin/bitcoin/pull/28892#discussion_r1399181655)
> MakeAndPushMessage
> https://github.com/ajtowns/bitcoin/commit/0fe129b1b437d39221cef843f969f1adc31b543e

Thanks, done both. Looks like you forgot `std::forward`? Added it, and added you as co-author

💬 maflcko commented on issue "fuzz, coinselection: Assertion 'result_bnb->GetChange(coin_params.m_cost_of_change, CAmount{0}) == 0' failed":
(https://github.com/bitcoin/bitcoin/issues/28918#issuecomment-1819037403)
Whatever is done here, at some point something needs to be done on the 26.x branch to avoid a failing CI.

💬 TheCharlatan commented on pull request "Fee Estimator updates from Validation Interface/CScheduler thread":
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1399186219)
Yes, I was only suggesting to remove the names, e.g. change `uint64_t mempool_sequence` to `uint64_t /*unused*/`.

💬 TheCharlatan commented on pull request "Fee Estimator updates from Validation Interface/CScheduler thread":
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1399189391)
Keeping around a dead branch for the purpose of fuzzing seems weird to me. Besides the `inBlock` case is still exercised from `processBlock`.

💬 maflcko commented on issue "fuzz, parse_iso8601: attempt to dereference an end-of-stream istreambuf_iterator":
(https://github.com/bitcoin/bitcoin/issues/28917#issuecomment-1819043199)
See https://github.com/boostorg/date_time/issues/190

💬 maflcko commented on issue "fuzz, parse_iso8601: attempt to dereference an end-of-stream istreambuf_iterator":
(https://github.com/bitcoin/bitcoin/issues/28917#issuecomment-1819049606)
> not sure why oss-fuzz doesn't find it.

oss-fuzz doesn't use gcc, but clang and libc++

💬 m3dwards commented on issue "26.0 RC Testing Guide Feedback":
(https://github.com/bitcoin/bitcoin/issues/28866#issuecomment-1819053244)
@kashifs thank you for your review, fixed all the found typos and removed the `$ ` signs.

Yes, for the mainnet test of import mempool you would need to be fully synced. I've added a note about that and an extra call to `-getinfo` to confirm it.

💬 Sjors commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1399223399)
0b284b5fd29c06d46c1ec60ea7e1bcd07f36feb1: can you order chunks by mining score?

💬 Sjors commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1399231122)
It would be good to explain the rationale for the miner score somewhere. Why a * b and b * a?

📝 hebasto opened a pull request: "build: Fix regression in "ARMv8 CRC32 intrinsics" test"
(https://github.com/bitcoin/bitcoin/pull/28919)
In the master branch, the `aarch64` binaries lack support for CRC32 intrinsics.

The `vmull_p64` is a part of the Crypto extensions from the ACLE. They are optional extensions, so they get enabled with a `+crypto` for architecture flags.

The regression was introduced in https://github.com/bitcoin/bitcoin/pull/26183 (v25.0).

The `./configure` script log excerpts:
- the master branch @ d752349029ec7a76f1fd440db2ec2e458d0f3c99:
```
checking whether C++ compiler accepts -march=armv8-a+crc
...

💬 sipa commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#issuecomment-1819105431)
@Sjors I believe most if not all of this PR will be rewritten, and split up into several components. The goal here is just to give an idea of the high-level interactions with other changes (wallet behavior, package validation/relay/RBF, ...). I don't think a detailed line-by-line code review at this stage is a good use of your time.