🤔 EthanHeilman reviewed a pull request: "Fixes compile errors in MSVC build #27332"
(https://github.com/bitcoin/bitcoin/pull/27335)

💬 EthanHeilman commented on pull request "Fixes compile errors in MSVC build #27332":
(https://github.com/bitcoin/bitcoin/pull/27335#discussion_r1159011140)
done

💬 jonatack commented on pull request "test: create random and coins utils, add amount helper, dedupe add_coin":
(https://github.com/bitcoin/bitcoin/pull/26940#issuecomment-1498148650)
> setup_common can't use any of the random functions without creating a circular dependency

Addressed with #27425.

💬 TheCharlatan commented on pull request "refactor: Extract common/args from util/system":
(https://github.com/bitcoin/bitcoin/pull/27419#issuecomment-1498153284)
RE https://github.com/bitcoin/bitcoin/pull/27419#pullrequestreview-1373289140

> Just an option to consider, but I think could be good to move GetConfigFile, GetConfigOptions, ArgsManager::ReadConfigStream, and ArgsManager::ReadConfigFiles functions to src/common/config.cpp instead of src/common/args.cpp, since in longer run, it would be nice if it would be nice if there were a src/common/settings.cpp file to parse the settings file, src/common/config.cpp to parse the config file, and src/comm
...

💬 TheCharlatan commented on pull request "refactor: Extract common/args from util/system":
(https://github.com/bitcoin/bitcoin/pull/27419#issuecomment-1498165963)
Updated c02e451882bb1d220b944af3b444d4b529ac351a -> de2546e672d0a2103228d777b35bfa6aab4881ee ([splitSystemArgs_1](https://github.com/TheCharlatan/bitcoin/commits/splitSystemArgs_1) -> [splitSystemArgs_2](https://github.com/TheCharlatan/bitcoin/commits/splitSystemArgs_2), [compare](https://github.com/TheCharlatan/bitcoin/compare/splitSystemArgs_1..splitSystemArgs_2)):
* Addressed @hebasto's [comment](https://github.com/bitcoin/bitcoin/pull/27419#discussion_r1158131412) adding a missing copyright
...

🤔 mzumsande reviewed a pull request: "Deprecate and remove BIP35 mempool p2p message"
(https://github.com/bitcoin/bitcoin/pull/27426)
> or anybody still actively using this message to find out more about their use-cases.

how about a mailing list post to reach a wider audience?

💬 willcl-ark commented on issue "-fallbackfee should apply to estimatesmartfee":
(https://github.com/bitcoin/bitcoin/issues/27415#issuecomment-1498202324)
@kylezs Could you program your test to use the `--fallbackfee` if it recieves the error in reponse, or as Antoine reponded to you there fill up your test mempool so that `estimatesmartfee` works as intended?

Accessing `--fallbackfee` from within the fee estimation rpc involves importing a lot of wallet code which woudl be preferable to avoid...

🤔 pablomartin4btc reviewed a pull request: "wallet, tests: Expand and test when the blank wallet flag should be un/set"
(https://github.com/bitcoin/bitcoin/pull/25634)
re-ACK https://github.com/bitcoin/bitcoin/commit/c5fb9a4dd97b47db0e52c497a757dff943b255b1

📝 dimitaracev opened a pull request: "validation: Replicate MinBIP9WarningHeight with MinBIP9WarningStartTime"
(https://github.com/bitcoin/bitcoin/pull/27427)
This PR addresses [comment](https://github.com/bitcoin/bitcoin/pull/27357#issuecomment-1497009569). Replaces `CChainParams::MinBIP9WarningHeight` with `CChainParams::MinBIP9WarningStartTime`. It is then used in `WarningBitsConditionChecker::BeginTime` to only check block headers that are newer.
This helps reduce the block headers that are being scanned for warnings.

cc @ajtowns

👋 jonatack's pull request is ready for review: "test: move remaining rand code from util/setup_common to util/random"
(https://github.com/bitcoin/bitcoin/pull/27425)

👍 hebasto approved a pull request: "Fixes compile errors in MSVC build #27332"
(https://github.com/bitcoin/bitcoin/pull/27335)
re-ACK 6a9a4d13b2b22632e0acd4f86f7bac238294ba42

💬 josibake commented on pull request "contrib: allow multi-sig binary verification v2":
(https://github.com/bitcoin/bitcoin/pull/27358#discussion_r1159666836)
confirmed this does fix the download selection filter

💬 MarcoFalke commented on pull request "ci: Run base install at most once":
(https://github.com/bitcoin/bitcoin/pull/27429#discussion_r1159667906)
Yes, `DANGER_RUN_CI_ON_HOST` is called that way to discourage people from trashing their system

💬 MarcoFalke commented on pull request "ci: Run base install at most once":
(https://github.com/bitcoin/bitcoin/pull/27429#discussion_r1159668508)
(This applies to all `CI_EXEC` lines, so I don't think anything is gained from mentioning it here)

🤔 TheCharlatan reviewed a pull request: "Add "warnings", deprecate "warning" in {create,load,unload,restore}wallet"
(https://github.com/bitcoin/bitcoin/pull/27279)
Approach ACK

💬 josibake commented on pull request "contrib: allow multi-sig binary verification v2":
(https://github.com/bitcoin/bitcoin/pull/27358#discussion_r1159670307)
nit: it would be nice to have it also print out the destination, e.g "downloading {binary_filename} to {destination}"

💬 glozow commented on pull request "Deprecate and remove BIP35 mempool p2p message":
(https://github.com/bitcoin/bitcoin/pull/27426#issuecomment-1499006887)
Concept ACK. I agree there should be a mailing list post.

💬 petertodd commented on pull request "Remove -mempoolfullrbf option":
(https://github.com/bitcoin/bitcoin/pull/26525#issuecomment-1499080469)
> > Anyway the data from [miningpool.observer/template-and-block](https://miningpool.observer/template-and-block) shows that blocks routinely include a dozen or so unexpected transactions. It's not clear if those are transactions not in miningpool.observer's mempoo, and thus relevant to compact blocksl; @0xB10C, can you clarify this point?
>
> Transactions listed under "Extra Transactions" are transactions that weren't in my mempool but the pool knew about. These are transactions my node woul
...

🤔 glozow reviewed a pull request: "wallet: when a block is disconnected, update transactions that are no longer conflicted"
(https://github.com/bitcoin/bitcoin/pull/27145)
Concept ACK

There is a missing dash in "Co-authored-by:" in the commit messages

💬 glozow commented on pull request "wallet: when a block is disconnected, update transactions that are no longer conflicted":
(https://github.com/bitcoin/bitcoin/pull/27145#discussion_r1159806134)
Sorry to bikeshed, but I think the naming is a bit hard to follow ("conflicted" vs "conflicting" don't immediately make sense to me, especially since both sets of transactions get conflicted at some point during the test). Might I suggest names that indicate which input(s) they spend, which node broadcasts them, and what "generation" they are? For example

`tx_AB_0_parent` instead of `conflicted`
`tx_A_1` instead of `conflicting_tx_A`
`tx_B_1` instead of `conflicting_tx_B`

`C` instead of
...