💬 naiyoma commented on pull request "test: Refactor fee calculation to remove satoshi_round function":
(https://github.com/bitcoin/bitcoin/pull/29566#issuecomment-2072581932)
> The C.I failure here is due to unused import @naiyoma
>
> ```
> {'-zmqpubhashblock', '-zmqpubsequencehwm', '-zmqpubsequence', '-zmqpubrawblock', '-zmqpubhashtxhwm', '-zmqpubhashblockhwm', '-includeconf', '-zmqpubhashtx', '-zmqpubrawtx', '-testdatadir', '-zmqpubrawblockhwm', '-zmqpubrawtxhwm'}
> test/functional/test_framework/util.py:8:1: F401 'decimal.ROUND_DOWN' imported but unused
> ^---- failure generated from lint-python.py
>
> ^---- ⚠️ Failure generated from lint-*.py scripts!
>
...

💬 fjahr commented on pull request "rpc: Avoid getchaintxstats invalid results":
(https://github.com/bitcoin/bitcoin/pull/29720#issuecomment-2072584005)
Concept ACK, could we add a test for this?

💬 maflcko commented on pull request "rpc: Avoid getchaintxstats invalid results":
(https://github.com/bitcoin/bitcoin/pull/29720#issuecomment-2072599127)
A test is already present in `test/functional/feature_assumeutxo.py`, see the diff. You can also test it by running this pull against a `bitcoind` compiled from master and observing the sanitizer crash and the test failure.

💬 maflcko commented on issue "AssumeUTXO Mainnet Readiness Tracking":
(https://github.com/bitcoin/bitcoin/issues/29616#issuecomment-2072603812)
Thanks for adding!

💬 fjahr commented on pull request "[DO NOT MERGE] testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2072640353)
> > I doubt many people do that.
>
> Two people raised the concern in this thread, so why would you doubt it?

"Many" is very relative but I think we probably would not see a market for trading testnet coins against bitcoin if that is something everyone can do as easily as setting a bitcore core node for example.

📝 vostrnad opened a pull request: "Remove redundant `-datacarrier` option"
(https://github.com/bitcoin/bitcoin/pull/29942)
The `-datacarrier` option is redundant, as disabling the relay and mining of transactions creating OP_RETURN outputs can be accomplished by setting `-datacarriersize=0`. Its removal was supported by several people in the discussion of [#27261](https://github.com/bitcoin/bitcoin/pull/27261) (e.g. https://github.com/bitcoin/bitcoin/pull/27261#issuecomment-1472404739, https://github.com/bitcoin/bitcoin/pull/27261#issuecomment-1581058082) and implemented in [#28130](https://github.com/bitcoin/bitcoi
...

💬 fjahr commented on pull request "rpc: Avoid getchaintxstats invalid results":
(https://github.com/bitcoin/bitcoin/pull/29720#discussion_r1576472588)
Not returning a value here might break downstream projects if they are unlucky since this wasn't optional before. Not sure if deprecation is necessary here but a more safe approach could be to just return 0 if it is unknown. I don't have a strong opinion on it but usually, we are pretty cautious with a change like this.

💬 maflcko commented on pull request "Remove redundant `-datacarrier` option":
(https://github.com/bitcoin/bitcoin/pull/29942#issuecomment-2072731630)
> Being a breaking change, this will of course need a release note. I'd recommend anyone concerned that they might accidentally start relaying and mining OP_RETURN transactions to proactively set `-datacarriersize=0` now.

Not sure about saving 4 lines of code for a breaking change. There are other redundant options such as `-signet` and `-chain=signet`, so I don't see why this one should be removed?

💬 achow101 commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1576483883)
Oops fixed.

💬 vostrnad commented on pull request "Remove redundant `-datacarrier` option":
(https://github.com/bitcoin/bitcoin/pull/29942#issuecomment-2072789861)
The main benefit really is not having two options with similar names that can do the same thing. As you yourself said in the linked comment:

> Having two ways to achieve the same is just confusing and can lead to issues.

Seems to me a one-time breaking change (with very little impact) is preferred to keeping the confusing set of options forever.

🤔 ismaelsadeeq reviewed a pull request: "test: Refactor fee calculation to remove satoshi_round function"
(https://github.com/bitcoin/bitcoin/pull/29566#pullrequestreview-2017660631)
utACK 870e71cb8e66232c98b821e38b472e3b8ab9c1c9 🛸

Should also update the PR Title!

💬 maflcko commented on pull request "rpc: Avoid getchaintxstats invalid results":
(https://github.com/bitcoin/bitcoin/pull/29720#discussion_r1576507502)
I've implemented this in fa032a5792c79b76b038827977c8a5eafd8e297b, but changed it after https://github.com/bitcoin/bitcoin/pull/29720#discussion_r1537955050 to faeb2056cf5c63af727994a6775d27a3e160ceed.

When evaluating whether this is a breaking change, I don't think it can affect any downstream projects, because Assume-Utxo is an incomplete, experimental, regtest-only feature. Also, if changing the `txcount` return value were to affect downstream projects, then b50554babdddf452acaa51bac757736
...

💬 glozow commented on pull request "feefrac: avoid explicitly computing diagram; compare based on chunks":
(https://github.com/bitcoin/bitcoin/pull/29757#issuecomment-2072809949)
reACK b22901d

💬 fjahr commented on pull request "rpc: Avoid getchaintxstats invalid results":
(https://github.com/bitcoin/bitcoin/pull/29720#discussion_r1576521230)
Yeah, changing the return value wouldn't be much of a concern, but a missing key has higher potential to crash something.

> When evaluating whether this is a breaking change, I don't think it can affect any downstream projects, because Assume-Utxo is an incomplete, experimental, regtest-only feature.

(*testnet/regtest-only) I'm pretty much with you here but [others might not be](https://github.com/bitcoin/bitcoin/pull/29612#pullrequestreview-1974818797) though the utxo set dump might also
...

💬 maflcko commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1576523097)
I guess `i+3` fails, because you access the memory past the end of the string(view) as a reference and then take a pointer to that.

A possible fix could be to use iterators instead of memory-access. Something like this, but not sure if it compiles, or is the best solution.

```suggestion
auto [_, ec] = std::from_chars(url_encoded.begin() + (i + 1), url_encoded.begin() + (i + 3), decoded_value, 16);
```

💬 fjahr commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1576524917)
Yeah, I just pushed this.

💬 maflcko commented on pull request "depends: add new LLVM debug macro":
(https://github.com/bitcoin/bitcoin/pull/29781#issuecomment-2072840743)
Looks like it worked in https://github.com/bitcoin/bitcoin/runs/24156835563

```
/msan/cxx_build/include/c++/v1/string_view:399: assertion __pos < size() failed: string_view[] index out of bounds
```

?

💬 maflcko commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1576531967)
```suggestion
auto [_, ec] = std::from_chars(url_encoded.data() + i + 1, url_encoded.data() + i + 3, decoded_value, 16);
```

Probably a wrong push?

💬 fjahr commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1576534925)
fixed, thanks

💬 maflcko commented on pull request "rpc: Avoid getchaintxstats invalid results":
(https://github.com/bitcoin/bitcoin/pull/29720#discussion_r1576536504)
> (*testnet/regtest-only)

A good catch. Looks like is in all networks, except for `main`, so I guess it is still fine.