💬 desirepl commented on issue "Bitcoin ignores datadir and blocksdir parameter in .conf":
(https://github.com/bitcoin/bitcoin/issues/27246#issuecomment-1470006043)
It's not the only GUI bug.
1. Start bitcoind -debuglogfile
2. It says it's default datadir is in %AppData%\Bitcoin and "using this data directory if no bitcoin.conf there (-qt.exe readr Registry value for default datadir in that case)
3. Place bitcoin.conf there and add two lines: datadir and blocksdir (out of sections, in global scope)
4. Start bitcoind -debuglogfile again
5. It says it's default datadir and using it the same as in 2; blocksdir is used as in .conf. Now it is ignoring datad
...

💬 stickies-v commented on issue "Feature request: alert PR author in case of CI failure":
(https://github.com/bitcoin/bitcoin/issues/27178#issuecomment-1470021680)
> Because notifying the author in case of a intermittent CI network error is entirely pointless if they can't re-run the task anyway.

I think having a "CI failed" label is not a bad idea, but I don't agree that notifying everyone is desirable. Afaik everyone can re-run the CI on _their own_ PRs, if they connect their github account to cirrus? At least I'm able to, and I don't think I have any special privileges. For example, on https://cirrus-ci.com/task/5819454748098560 I have this option:

...

💬 vasild commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1137099484)
Where?

📝 petertodd opened a pull request: "Ignore datacarrier limits for dataless OP_RETURN outputs"
(https://github.com/bitcoin/bitcoin/pull/27261)
They don't carry any data, so the -datacarrier/-datacarriersize options should not apply to them. Previously a transaction containing an OP_RETURN without data would be rejected if datacarrier was set to false, or the datacarriersize was set to zero.

💬 MarcoFalke commented on issue "Feature request: alert PR author in case of CI failure":
(https://github.com/bitcoin/bitcoin/issues/27178#issuecomment-1470067183)
I am mostly thinking that merely assigning the author or applying a label does not provide enough information. Only pressing the re-run button is rarely the right choice (correct me if I am wrong):

* Intermittent Network failure (should be rare) -> Re-run (if "author" and "member"), else ask a maintainer for re-run.
* Pre-existing test failure -> Search/File issue, then Re-run.
* Test failure introduced in the pull -> Author needs to fix it.

But I guess, if the Label workflow is chosen,
...

💬 instagibbs commented on pull request "Ignore datacarrier limits for dataless OP_RETURN outputs":
(https://github.com/bitcoin/bitcoin/pull/27261#issuecomment-1470071077)
Is the use-case idea to be able to a little more compactly burn a dusty output to OP_RETURN?

💬 petertodd commented on pull request "Ignore datacarrier limits for dataless OP_RETURN outputs":
(https://github.com/bitcoin/bitcoin/pull/27261#issuecomment-1470081403)
> Is the use-case idea to be able to a little more compactly burn a dusty output to OP_RETURN?

Yes. For example, my dust-b-gone script uses a bare OP_RETURN output without data: https://github.com/petertodd/dust-b-gone/blob/95020b4d0280d5ad2c8973b2e981e5dd966e2315/merge-dust-txs.py#L70

To be clear, bare OP_RETURN is currently standard with the default datacarrier settings. All this pull-req changes is it'll allow a bare OP_RETURN in the (rare) case that people are disabling datacarrier out
...

💬 TheCharlatan commented on pull request "refactor / kernel: Move non-gArgs chainparams functionality to kernel":
(https://github.com/bitcoin/bitcoin/pull/26177#issuecomment-1470084204)
re https://github.com/bitcoin/bitcoin/pull/26177#pullrequestreview-1341271390
> Please advise maintainers if this should be merged and if you plan to address the nits (if any).

Yes, will address the nits. It think it's worth another round.

💬 MarcoFalke commented on pull request "refactor / kernel: Move non-gArgs chainparams functionality to kernel":
(https://github.com/bitcoin/bitcoin/pull/26177#issuecomment-1470098332)
Ok, but please don't address those that are marked "unrelated", as that would not be refactoring changes, but behavior changes/bugfixes.

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