💬 TheCharlatan commented on pull request "refactor / kernel: Move non-gArgs chainparams functionality to kernel":
(https://github.com/bitcoin/bitcoin/pull/26177#discussion_r1137163135)
This produces a log like:
```
terminate called after throwing an instance of 'std::runtime_error'
what(): CreateChainParams: Unknown chain lol
```
So I think yes, it does add value.

💬 desirepl commented on issue "Bitcoin ignores datadir and blocksdir parameter in .conf":
(https://github.com/bitcoin/bitcoin/issues/27246#issuecomment-1470110239)
bitcoin_wallet -datadir=*** info: "Failed to load database path '***'. Data is not in recognized format."
*** = any valid datadir/blocksdir/walletdir (only one wallet is in sqlite format, regtest)
bitcoin_wallet searches wallets on datadir? but for other tools there are walletdir parameter...

💬 vasild commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1137181909)
I think so, thanks!

👍 vasild approved a pull request: "addrman: Enable selecting addresses by network"
(https://github.com/bitcoin/bitcoin/pull/27214)
ACK 09d514583f15860f3bc7ae0c89e640c94fae3c71

Suggestions remaining (non-blocker):

* [Check out-of-bounds array access in AddrManImpl::GetEntry()](https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1129439158).

* I can't find where the [suggested test](https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1130705627) was added.

* Performance wise I think the change in https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1130705627 would be very nice to have.

Thanks!
...

💬 bigspider commented on pull request "MiniTapscript: port Miniscript to Tapscript":
(https://github.com/bitcoin/bitcoin/pull/27255#discussion_r1137201661)
It might be worth explicitly enumerating both the known contexts, and have a defensive final `assert(false);`.
Similarly for other places where the context is accessed).

💬 MarcoFalke commented on pull request "Remove almost all blockstorage globals":
(https://github.com/bitcoin/bitcoin/pull/25781#discussion_r1137211913)
Yes, makes sense for consistency. Thanks, done.

> This also similarly applies to fPruneMode.

I don't think it does. I've reworked the second commit.

💬 darosior commented on pull request "MiniTapscript: port Miniscript to Tapscript":
(https://github.com/bitcoin/bitcoin/pull/27255#discussion_r1137224005)
I could have a `IsTapscript(ctx)` helper that does that next to the `MiniscriptContext` enum. It could then be used in all those places to not have to duplicate the verbose `switch () ... assert(false)`.

💬 Xekyo commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#issuecomment-1470172234)
Rebased on latest version of https://github.com/bitcoin/bitcoin/pull/27021. If you are interested in having #26152 in Bitcoin Core v25.0, please consider making time to review #27021.

💬 MarcoFalke commented on pull request "Remove almost all blockstorage globals":
(https://github.com/bitcoin/bitcoin/pull/25781#issuecomment-1470172489)
I've added a new commit, because all of this was reworked anyway.

💬 bigspider commented on pull request "MiniTapscript: port Miniscript to Tapscript":
(https://github.com/bitcoin/bitcoin/pull/27255#discussion_r1137247463)
Why not this?
```C++
if (ctx.MsContext() == MiniscriptContext::P2WSH) {
return GetStackSize() <= MAX_STANDARD_P2WSH_STACK_ITEMS;
}
```
Otherwise, it seems to add the other condition for the `P2WSH` context as well.

📝 rebroad opened a pull request: "Refine wallet synchronization warning for enhanced clarity"
(https://github.com/bitcoin/bitcoin/pull/27262)
This pull request updates the wallet synchronization warning text to provide a clearer and more concise message to users. The changes made are as follows:

- Replace the original warning text with a more concise version.
- Clarify that transactions using bitcoin from unseen transactions due to ongoing synchronization will be rejected by the network.

These changes aim to improve user experience by providing clearer information about potential issues related to wallet synchronization.

Old
...

💬 bigspider commented on pull request "MiniTapscript: port Miniscript to Tapscript":
(https://github.com/bitcoin/bitcoin/pull/27255#discussion_r1137259200)
Typo: "contexts"

💬 Xekyo commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#issuecomment-1470194229)
I’ve run both of the fuzz targets for 48-CPU-hours last night, and have not seen another crash bug.

💬 fanquake commented on pull request "Refine wallet synchronization warning for enhanced clarity":
(https://github.com/bitcoin/bitcoin/pull/27262#issuecomment-1470204188)
I'm not sure your changes are actually an improvement. However, you can drop all of the changes you've made to the .ts files. You've also opened this in the wrong repository. It's a GUI change.

💬 bigspider commented on pull request "MiniTapscript: port Miniscript to Tapscript":
(https://github.com/bitcoin/bitcoin/pull/27255#discussion_r1137280634)
Nit: missing a newline

✅ rebroad closed a pull request: "Refine wallet synchronization warning for enhanced clarity"
(https://github.com/bitcoin/bitcoin/pull/27262)

💬 rebroad commented on pull request "Refine wallet synchronization warning for enhanced clarity":
(https://github.com/bitcoin/bitcoin/pull/27262#issuecomment-1470224943)
> I'm not sure your changes are actually an improvement. However, you can drop all of the changes you've made to the .ts files. You've also opened this in the wrong repository. It's a GUI change.

It's mostly motivated by the poor grammar of the current wording. We should at least make it grammatically correct, shouldn't we? Are you objecting to it now being so few words in the first part? I can leave that unchanged if so.

💬 bigspider commented on pull request "MiniTapscript: port Miniscript to Tapscript":
(https://github.com/bitcoin/bitcoin/pull/27255#discussion_r1137296790)
I suppose it never happens, but worth checking that `a.value >= b`?
Especially since it could be used with unsigned types.

💬 brunoerg commented on pull request "test: fix intermittent issue in `feature_bip68_sequence`":
(https://github.com/bitcoin/bitcoin/pull/27177#discussion_r1137297128)
Yes, you're right, there is a way to simplify it, no need to rewrite the filter. Force-pushed addressing it.

📝 rebroad opened a pull request: "Correct poor grammar in wallet synchronization warning."
(https://github.com/bitcoin-core/gui/pull/720)
This pull request updates the wallet synchronization warning text to provide a clearer and more concise message to users. The original text was somewhat clumsy and suffered from grammatical issues, which could lead to confusion. The changes made are as follows:

- Replace the original warning text with a more concise and grammatically correct version.
- Clarify that transactions using bitcoin from unseen transactions due to ongoing synchronization will be rejected by the network.

These cha
...