💬 VojtechMyslivec commented on issue "gettransaction details does not include send to myself balance changes for imported addresses":
(https://github.com/bitcoin/bitcoin/issues/28555#issuecomment-2236269937)
If you send a transaction from one of your address to another one in the same wallet, sum of `amount`s would be zero and in details, you would see one _receiving_ and one _sending_ balance change.

This is one of my example transaction:

```
{
"amount": 0.00000000,
"fee": -0.00004200,
...
"txid": "...",
...
"details": [
{
"address": "...y",
"category": "send",
"amount": -0.00012000,
"fee": -0.00004200,
...
},
{
"addr
...

🤔 glozow reviewed a pull request: "Fee Estimation: change `estimatesmartfee` default mode to `economical`"
(https://github.com/bitcoin/bitcoin/pull/30275#pullrequestreview-2185560501)
ACK 41a2545046bce315af697a3c6baf6e3fb2e824c2

Checked that default `estimatesmartfee` result is using economical.

💬 maflcko commented on pull request "test, refactor: Fix MSVC warning C4101 "unreferenced local variable"":
(https://github.com/bitcoin/bitcoin/pull/30464#issuecomment-2236275618)
ACK 44f08786f435ed4284d39dc604c2a5fcbde9e602

💬 maflcko commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1682689533)
Yes, that is unrelated and deterministic tests should be added.

💬 Eunovo commented on pull request "Tr partial descriptors":
(https://github.com/bitcoin/bitcoin/pull/30243#discussion_r1682690671)
No it doesn't. I have changed the type to `uint8_t`

💬 Eunovo commented on pull request "Tr partial descriptors":
(https://github.com/bitcoin/bitcoin/pull/30243#discussion_r1682693031)
In https://github.com/bitcoin/bitcoin/pull/30243/commits/0b30b690e7067240b05ece17fdc7ad44549368a3 I now use `TryParseHex` and just check for the presence of a response

💬 maflcko commented on pull request "qa: Functional test improvements":
(https://github.com/bitcoin/bitcoin/pull/30463#discussion_r1682699162)
Ok, so it can be reproduced in Linux with hard links.

However, this leads back to my original question: Why not change the other places as well?

For me, they also fail:

```
$ ln $PWD/../test/functional/p2p_dos_header_tree.py ./test/functional/
$ ./test/functional/p2p_dos_header_tree.py
2024-07-18T11:42:08.340000Z TestFramework (INFO): PRNG seed is: 5276922920789009906
2024-07-18T11:42:08.341000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_9ppp3fwq
...

👋 paplorinc's pull request is ready for review: "optimization: Precalculate SipHash constant XOR with k0 and k1 in SaltedOutpointHasher"
(https://github.com/bitcoin/bitcoin/pull/30442)

🚀 fanquake merged a pull request: "test, refactor: Fix MSVC warning C4101 "unreferenced local variable""
(https://github.com/bitcoin/bitcoin/pull/30464)

💬 fanquake commented on pull request "depends: remove Darwin ENV unsetting":
(https://github.com/bitcoin/bitcoin/pull/30451#issuecomment-2236326039)
Assuming that #29723 lands first, the commit here adding a patch for librt will be dropped, as that is also patched out of the CMake side in that PR.

👍 ismaelsadeeq approved a pull request: "refactor: add coinbase constraints to BlockAssembler::Options"
(https://github.com/bitcoin/bitcoin/pull/30356#pullrequestreview-2185605742)
Code review ACK c504b6997b1acc9771ad1f52efaa4be2b4966c6c

The PR description needs to be updated to match the PR.

Left a single question

💬 ismaelsadeeq commented on pull request "refactor: add coinbase constraints to BlockAssembler::Options":
(https://github.com/bitcoin/bitcoin/pull/30356#discussion_r1682712434)
Is their a reason we are not adding `coinbase_script` into the `BlockCreateOptions` struct?

💬 craigraw commented on issue "Feature Request: Broadcast Pool":
(https://github.com/bitcoin/bitcoin/issues/30471#issuecomment-2236343826)
> As a user, I'd probably expect to be notified that a transaction isn't in "the connected mempool" and be offered some kind of optional fee-bump (or rebroadcast button, if the transaction hit mempool-expiry).

In general I agree. I was referring specifically to future-dated transactions there, so we may be speaking slightly at cross purposes. But to speak to broadcasted transactions, it's less important to consider the local mempool, since we must assume they are contained in some miners memp
...

💬 hodlinator commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1682745543)
I agree `uint256::FromHex()` is more self-explanatory than `uint256S()`. Making it return `std::optional` might be good if we want stricter parsing and non-fatal failures. This PR feels large enough for now though.

💬 maflcko commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1682755930)
Yes, all of this should happen later, because it is unrelated to fixing possible UB/crashes (the goal of this pull).

💬 hodlinator commented on pull request "refactor: Make uint256S(const char*) and ParseHex(const char*) consteval":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1682767232)
Fixed as of 10baae11ecc376f2250d4a51d2bbfeea56c0a31d.

💬 paplorinc commented on pull request "fuzz: Speed up PickValue in txorphan":
(https://github.com/bitcoin/bitcoin/pull/30474#discussion_r1682771932)
Since a random access vector seems to resolve the problem, is there any reason we cannot change the `outpoints` definition to a vector instead?

💬 hodlinator commented on pull request "refactor: Make uint256S(const char*) and ParseHex(const char*) consteval":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1682772285)
As of 10baae11ecc376f2250d4a51d2bbfeea56c0a31d I have stopped overloading `uint256S()`:

Changed from `consteval uint256 uint256S(const char *str)` -> `consteval explicit uint256(const char (&str)[65])` and `base_blob(std::string_view str)`
This means the string width is enforced at compile time. (The new parsing code is much more strict and also asserts on the length).

Applied uint256S -> uint256 conversion where applicable, removing "0x"-prefixes.
TODO: if there is agreement on the curr
...

💬 hodlinator commented on pull request "refactor: Make uint256S(const char*) and ParseHex(const char*) consteval":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1682773691)
More strict as of 10baae11ecc376f2250d4a51d2bbfeea56c0a31d.

💬 hodlinator commented on pull request "refactor: Make uint256S(const char*) and ParseHex(const char*) consteval":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1682775910)
`consteval` parsing now done inside `base_blob(std::string_view)` as of 10baae11ecc376f2250d4a51d2bbfeea56c0a31d.