💬 hebasto commented on issue "ci: Future of macOS and Windows MSVC CI tasks":
(https://github.com/bitcoin/bitcoin/issues/28098#issuecomment-1651432106)
> I think before working on GitHub Actions CI, it should be clear how `GITHUB_TOKEN` behaves inside GitHub CI and whether it allows code pushes from the CI or from pull requests to the main branch.

GitHub Action docs have a plenty of hints about that:
- https://docs.github.com/en/actions/security-guides/automatic-token-authentication#permissions-for-the-github_token
- https://securitylab.github.com/research/github-actions-preventing-pwn-requests/

💬 brunoerg commented on pull request "fuzz: add target for `ScriptPubKeyMan` (legacy)":
(https://github.com/bitcoin/bitcoin/pull/28153#discussion_r1274710779)
Yea, I just realized it. I'm changing it!

💬 MarcoFalke commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1274712377)
Apologies for missing this. It is an undocumented requirement that the functional usdt tests are run as root (not sure why). So this will disable them.

I think this should either be reverted, or the usdt requirement for root be dropped.

💬 TheCharlatan commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1274723895)
https://github.com/bitcoin/bitcoin/pull/28162

💬 TheCharlatan commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1274724027)
https://github.com/bitcoin/bitcoin/pull/28162

💬 MarcoFalke commented on issue "ci: Future of macOS and Windows MSVC CI tasks":
(https://github.com/bitcoin/bitcoin/issues/28098#issuecomment-1651450188)
Ok, so I presume "Maximum access for pull requests from public forked repositories" applies and no further action is needed? Or maybe recommend that in the org (or repo) settings the permission is lowered to `restricted` just to be safe?

💬 petertodd commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1651454924)
> Concept ACK, but the description has some issues:
>
> > Obviously, with this much support of full-rbf, arguments against it on the basis that unconfirmed transactions are safe are now invalid.
>
> It was always invalid.

Reworded it to say "even more clearly invalid"

> > Secondly, on the basis of mempool consistency with miners, since full-rbf is the compatible policy in almost all cases, we should be supporting it by default if you take the position that we want to optimize for con
...

💬 hebasto commented on issue "ci: Future of macOS and Windows MSVC CI tasks":
(https://github.com/bitcoin/bitcoin/issues/28098#issuecomment-1651455731)
> Or maybe recommend that in the org (or repo) settings the permission is lowered to `restricted` just to be safe?

Good idea.

💬 stickies-v commented on pull request "refactor: consistently use ApplyArgsManOptions for PeerManager::Options":
(https://github.com/bitcoin/bitcoin/pull/28148#discussion_r1274732379)
Agreed, makes more sense. Will move down if I have to retouch.

💬 stickies-v commented on pull request "refactor: consistently use ApplyArgsManOptions for PeerManager::Options":
(https://github.com/bitcoin/bitcoin/pull/28148#issuecomment-1651491901)
> I'm not sure that I love using a subsystem's opts as a cache for init vars.

I felt iffy about it too ([as per my initial suggestion](https://github.com/bitcoin/bitcoin/pull/27499#discussion_r1268400386)), it's not the most elegant. I do think using a single var is safer and more clear than having a separate `ignores_incoming_txs` var, but I'm happy to drop the second commit if that's what people prefer.

💬 fanquake commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#issuecomment-1651515949)
> @fanquake could you provide more info on the methodology to reproduce your data?

@jonatack I'm just running the preprocessor and comparing the to-be-compiled output. You could inject it into the compile command from `V=1`. i.e something like `/usr/bin/ccache g++ -std=c++17 -DHAVE_CONFIG_H -I. -I../src/config -fmacro-prefix-map=/home/ubuntu/bitcoin=. -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3 -DHAVE_BUILD_INFO -DPROVIDE_FUZZ_MAIN_FUNCTION -I. -I./minisketch/include -I./secp256k1/include -I./univ
...

💬 MarcoFalke commented on pull request "refactor: consistently use ApplyArgsManOptions for PeerManager::Options":
(https://github.com/bitcoin/bitcoin/pull/28148#issuecomment-1651531909)
You could add back the `const bool ignores_incoming_txs{peerman_opts.ignore_incoming_txs};` alias? But I don't see what is iffy about the code. Are you saying it would be cleaner for `node.fee_estimator` to be constructed after the peerman, and using the peerman property instead of the peerman options property?

🤔 stickies-v reviewed a pull request: "refactor: Revert additional univalue check in ParseSighashString"
(https://github.com/bitcoin/bitcoin/pull/28162#pullrequestreview-1547452713)
Approach ACK on the release notes updates (nice catch MarcoFalke), not so sure about the code changes yet.

💬 stickies-v commented on pull request "refactor: Revert additional univalue check in ParseSighashString":
(https://github.com/bitcoin/bitcoin/pull/28162#discussion_r1274785751)
I don't think catching runtime errors here is the best approach. We never expect this to fail at runtime, only if the function is used the wrong way by a programmer. Will this not unnecessarily hide true bugs?

I think avoiding dead code makes sense, but I also like the function being self-contained and self-descriptive like this without too much overhead, so I guess overall I'm ~0 on the change.

As an alternative, how about instead of catching `std::runtime_error&`, we only call `ParseSigh
...

💬 dergoegge commented on pull request "refactor: consistently use ApplyArgsManOptions for PeerManager::Options":
(https://github.com/bitcoin/bitcoin/pull/28148#discussion_r1274865617)
No strong opinion, but it would be perfectly fine to just do
```suggestion
if (!args.GetBoolArg("-blocksonly", DEFAULT_BLOCKSONLY)) {
```
here and then keep the peerman opts were they are right now.

💬 hebasto commented on issue "ci: Future of macOS and Windows MSVC CI tasks":
(https://github.com/bitcoin/bitcoin/issues/28098#issuecomment-1651688197)
> Or maybe recommend that in the org (or repo) settings the permission is lowered to `restricted` just to be safe?

Here:

- Check "Read repository contents and packages permissions
- Uncheck "Allow GitHub Actions to create and approve pull requests

💬 dergoegge commented on pull request "util: Type-safe transaction identifiers":
(https://github.com/bitcoin/bitcoin/pull/28107#issuecomment-1651744555)
> Concept ACK, but I'm not too sure about the Wtxid type.

Could you clarify your objections about the `Wtxid` type? This PR aims to catch type confusion bugs (confusion between txids and wtxids) at compile-time and I don't see how to do that without these explicit types.

💬 MarcoFalke commented on pull request "test: Avoid intermittent issues due to async events in validationinterface_tests":
(https://github.com/bitcoin/bitcoin/pull/28150#issuecomment-1651767856)
Interestingly, I can *not* reproduce this easily with `rr`. I tried:

```
while rr record --chaos ./src/test/test_bitcoin -t validationinterface_tests/unregister_all_during_call ; do true ; done

💬 MarcoFalke commented on issue "Bitcoin Core v25.0 Crashes":
(https://github.com/bitcoin/bitcoin/issues/28119#issuecomment-1651776726)
Ok, closing for now. Let us know if this should be reopened.

✅ MarcoFalke closed an issue: "Bitcoin Core v25.0 Crashes"
(https://github.com/bitcoin/bitcoin/issues/28119)