💬 laanwj commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1576327472)
CI linter complains, apparently the option needs to be added in another place:
```
AssertionError: Please add {'-swapbdbendian'} to the hidden args in DummyWalletInit::AddWalletOptions
```

💬 fjahr commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1576348255)
should be fixed now, weird, I must have some plugin that inserts these tabs :/

💬 glozow commented on pull request "test: add MiniWallet tagging support to avoid UTXO mixing, use in `fill_mempool`":
(https://github.com/bitcoin/bitcoin/pull/29939#issuecomment-2072442023)
Nice, concept ACK

💬 laanwj commented on issue "guix: SOURCE_DATE_EPOCH is already set in some environments":
(https://github.com/bitcoin/bitcoin/issues/29935#issuecomment-2072443037)
Sounds good to me. I think it's fine to rename it, I think there's really very few edge cases in which one would really want to pass in a custom epoch to the guix build?

🤔 BrandonOdiwuor reviewed a pull request: "test: create assert_not_equal util"
(https://github.com/bitcoin/bitcoin/pull/29500#pullrequestreview-2017441166)
Approach ACK

Good job approaching this using Scripted diffs

💬 instagibbs commented on pull request "test: add missing comparison of node1's mempool in MempoolPackagesTest":
(https://github.com/bitcoin/bitcoin/pull/29941#discussion_r1576377434)
these comments are not needed(and indented wrong); I think they can just all be removed since the test is self-explanatory

💬 fjahr commented on pull request "[DO NOT MERGE] testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2072531117)
> ive currently they would need to wait for it to become active and that could take weeks, right? And when it becomes active I would imagine the miner who found the first block in the difficulty=1 series just blasts the network and the CPU miner still has no chance to get a block in between.

I raised my doubts about this actually being realistic [here](https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2041139129) and there was no response to it. Have you solo-mined testnet3 blocks on
...

💬 brunoerg commented on pull request "fuzz: wallet: add target for `CreateTransaction`":
(https://github.com/bitcoin/bitcoin/pull/29936#discussion_r1576391155)
Done. I've created a util file for wallet into `test/fuzz/util`.

💬 stratospher commented on pull request "test: Extends wait_for_getheaders so a specific block hash can be checked":
(https://github.com/bitcoin/bitcoin/pull/29736#discussion_r1576392771)
oh makes sense! `wait_for_getheaders` is done on the peer which we made sure didn’t receive getheaders before. i'm fine with leaving it as it is.

🤔 stratospher reviewed a pull request: "test: Extends wait_for_getheaders so a specific block hash can be checked"
(https://github.com/bitcoin/bitcoin/pull/29736#pullrequestreview-2017489889)
tested ACK c4f857c. went through all getheaders messages sent in the tests and checked that it's the one we want.

💬 fjahr commented on pull request "[DO NOT MERGE] testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2072550269)
> Yes, I am not sure what would be the problem. All you have to do is to set the time +20min and mine a block on your laptop. If you don't want to try it yourself, you can come by to watch it on my laptop.

I missed that response, so if this is possible at any time with or without a block storm happening, I am not sure how the change here is making a difference? I will give it a try.

💬 fjahr commented on issue "AssumeUTXO Mainnet Readiness Tracking":
(https://github.com/bitcoin/bitcoin/issues/29616#issuecomment-2072564502)
> Why is #29720 / #29328 not mentioned here?

Because nobody else mentioned them here so far, tagged this issue in either or notified me about them.

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