⚠️ MarcoFalke opened an issue: "error: in "validationinterface_tests/unregister_all_during_call": check destroyed has failed"
(https://github.com/bitcoin/bitcoin/issues/28146)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

check failed

### Expected behaviour

not fail

### Steps to reproduce

?

### Relevant log output

https://cirrus-ci.com/task/4769037708689408?logs=ci#L3768

```
test/validationinterface_tests.cpp(93): error: in "validationinterface_tests/unregister_all_during_call": check destroyed has failed
```



### How did you obtain Bitcoin Core

Compiled from source

### What version of Bitc
...

💬 MarcoFalke commented on issue "error: in "validationinterface_tests/unregister_all_during_call": check destroyed has failed":
(https://github.com/bitcoin/bitcoin/issues/28146#issuecomment-1649540097)
`TestInterface::Call();` should be blocking on the destructor, which sets `destroyed`, so I have no idea how this could happen.

💬 MarcoFalke commented on pull request "net processing, refactor: Decouple PeerManager from gArgs":
(https://github.com/bitcoin/bitcoin/pull/27499#discussion_r1273346189)
You forgot to read `-blocksonly` in this function

💬 fanquake commented on pull request "test: Bump walletpassphrase timeout to avoid intermittent issue":
(https://github.com/bitcoin/bitcoin/pull/28089#issuecomment-1649599987)
I think this PR can now be closed given #28139?

📝 fanquake converted_to_draft a pull request: "build: pass sanitize flags to instrument libsecp256k1 code"
(https://github.com/bitcoin/bitcoin/pull/27991)
secp256k1 functions are now instrumented:
```bash
# objdump --disassemble-symbols=_secp256k1_xonly_pubkey_serialize src/test/fuzz/fuzz | grep sanitizer_cov
100eb5244: 73 5d 01 94 bl 0x100f0c810 <___sanitizer_cov_trace_const_cmp8>
100eb53f0: cc 5c 01 94 bl 0x100f0c720 <___sanitizer_cov_trace_pc_indir>
100eb5418: c2 5c 01 94 bl 0x100f0c720 <___sanitizer_cov_trace_pc_indir>
100eb5444: b7 5c 01 94 bl 0x100f0c720 <___sanitizer_cov_trace_pc_indir>
```

Not sure if this is the best solutio
...

💬 stickies-v commented on pull request "net processing, refactor: Decouple PeerManager from gArgs":
(https://github.com/bitcoin/bitcoin/pull/27499#discussion_r1273361119)
I think this is on purpose since we use `ignore_incoming_txs` for multiple purposes, see also my suggested diff in https://github.com/bitcoin/bitcoin/pull/27499#discussion_r1268400386 (although I now see it can be simplified even further by not passing `ignores_incoming_txs` to the `Options` constructor at all). So, I don't think it's forgotten, but room for improvement in follow-up? Happy to open a pull for this if you think it's an improvement.

💬 fanquake commented on pull request "doc: cleanup release process doc":
(https://github.com/bitcoin/bitcoin/pull/28003#discussion_r1273370949)
Added, but had to use `-m` for macOS users.

💬 MarcoFalke commented on pull request "net processing, refactor: Decouple PeerManager from gArgs":
(https://github.com/bitcoin/bitcoin/pull/27499#discussion_r1273370954)
Yeah, I don't see what is wrong with your diff (apart from it not compiling).

I just don't get the point of adding the `ApplyArgsManOptions` helper when it is not used consistently. Might as well not add it in the first place?

💬 MarcoFalke commented on pull request "doc: cleanup release process doc":
(https://github.com/bitcoin/bitcoin/pull/28003#discussion_r1273372415)
My assumption was to run the `git` command on the machine that is running guix. I don't think macOS can run guix?

💬 MarcoFalke commented on pull request "net, refactor: extract Network and BIP155Network logic to node/network":
(https://github.com/bitcoin/bitcoin/pull/27385#issuecomment-1649628432)
> which may reduce build size and speed up build times.

May be good to add numbers?

💬 MarcoFalke commented on pull request "test: Bump walletpassphrase timeout to avoid intermittent issue":
(https://github.com/bitcoin/bitcoin/pull/28089#issuecomment-1649629599)
Thanks, yes.

✅ MarcoFalke closed a pull request: "test: Bump walletpassphrase timeout to avoid intermittent issue"
(https://github.com/bitcoin/bitcoin/pull/28089)

💬 pinheadmz commented on pull request "net: Add new permission `forceinbound` to evict a random unprotected connection if all slots are otherwise full":
(https://github.com/bitcoin/bitcoin/pull/27600#issuecomment-1649649475)
Thanks so much for taking a look Gleb and for your insight, yeah I have the same concerns. That attack vector is not super trivial, the attacker needs to both learn and then spoof the protected IP range. The release notes could recommend only using ranges for local subnets, or we could enforce that in the code. We could also change it so`forceinbound` does not evict existing peers but *may* add connections beyond the maximum, or set another internal limit just for `forceinbound`

💬 TheCharlatan commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1273422669)
Seems like `std::runtime_error` is still thrown: https://github.com/TheCharlatan/bitcoin/commits/kernelRmUnivalue_8.

👋 dergoegge's pull request is ready for review: "fuzz: Test headers pre-sync through p2p interface"
(https://github.com/bitcoin/bitcoin/pull/28043)

💬 dergoegge commented on pull request "fuzz: Test headers pre-sync through p2p interface":
(https://github.com/bitcoin/bitcoin/pull/28043#issuecomment-1649733407)
Rebased and un-drafted, ready for review!

💬 dergoegge commented on pull request "fuzz: Test headers pre-sync through p2p interface":
(https://github.com/bitcoin/bitcoin/pull/28043#discussion_r1273450317)
My thinking was that this should result in more coverage because we'll also cover paths for invalid PoW.

💬 TheCharlatan commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1273454021)
Mmh, we don't do this for any of the other `get_str()` calls, so this might be better for a follow up? Would probably also be good to re-use the text from the type error.

💬 jonatack commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1273460013)
Perhaps move that check back to the callee until the sighash enum is extracted.

💬 MarcoFalke commented on pull request "doc: cleanup release process doc":
(https://github.com/bitcoin/bitcoin/pull/28003#discussion_r1273461027)
But then it shouldn't be included in the git commit. If you run guix in a VM, my thinking was to include the architecture of the VM/guix, not the one of the macOS or BSD machine.