📝 fanquake opened a pull request: "valgrind: add suppression for bug 472219"
(https://github.com/bitcoin/bitcoin/pull/28145)
Now that https://bugs.kde.org/show_bug.cgi?id=472219 has been fixed upstream in:

https://sourceware.org/git/?p=valgrind.git;a=commit;h=6ce0979884a8f246c80a098333ceef1a7b7f694d

Add a supression to ignore the bug until we are using a fixed version of Valgrind.

Related to #28072.

💬 dergoegge commented on pull request "net processing, refactor: Decouple PeerManager from gArgs":
(https://github.com/bitcoin/bitcoin/pull/27499#issuecomment-1649465874)
> Maybe as a followup we could add some pedantic checks in init to make sure they're sane values that fit into 32bits and make max_orphan_txs a uint32_t. Please ignore if those checks exist and I'm not seeing them.

Thanks for pointing this out. I agree that some sane limits/checks would make sense for these settings as users could shoot themselves in the foot otherwise. I will leave this for a follow-up though as this isn't a new problem.

💬 MarcoFalke commented on pull request "valgrind: add suppression for bug 472219":
(https://github.com/bitcoin/bitcoin/pull/28145#issuecomment-1649478876)
lgtm ACK 50f7214e0915a88dd81c1ac1d292e049a398cda2

Didn't test

💬 naumenkogs commented on pull request "p2p: make block download logic aware of limited peers threshold":
(https://github.com/bitcoin/bitcoin/pull/28120#issuecomment-1649479100)
> What would be bad is if all our current peers were limited, and we wouldn't try to request the needed blocks from anyone, but also wouldn't try to exchange them for better peers, so that we would be stuck.

I agree with this part. I think the solution should be expanded to give a chance in such cases, and it should be merged within the same PR.

💬 naumenkogs commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#issuecomment-1649481055)
ACK f5d17e1a3f2856483ad3a6abcf3ea415f2d05fde

🚀 fanquake merged a pull request: "net processing, refactor: Decouple PeerManager from gArgs"
(https://github.com/bitcoin/bitcoin/pull/27499)

👍 fanquake approved a pull request: "fuzz: Re-enable symbolize=1 in ASAN_OPTIONS"
(https://github.com/bitcoin/bitcoin/pull/28124#pullrequestreview-1545063455)
ACK faa8c1be265d2344a3bc0932455b0182ec7d64c7

🚀 fanquake merged a pull request: "fuzz: Re-enable symbolize=1 in ASAN_OPTIONS"
(https://github.com/bitcoin/bitcoin/pull/28124)

💬 stickies-v commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1273298422)
I think this catch now needs to go?

💬 naumenkogs 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-1649517478)
Consider when a malicious Internet provider or something can request the `forceinbound` IP address.

Does this PR open any fundamentally new risk in this case? I'm thinking of disrupting the network by causing many disconnections if the `forceinbound` practice is broad.

This probably depends on whether it's possible to take multiple slots from the same `forceinbound` IP (or, rather, initiate multiple evictions), which I'm not 100% sure about.

If my suspicions are correct, this might bet
...

💬 MarcoFalke commented on pull request "ci: Start with clean env":
(https://github.com/bitcoin/bitcoin/pull/27976#issuecomment-1649519608)
Split out an easy-to-test bugfix: https://github.com/bitcoin/bitcoin/pull/28138 . So please review that one first.

💬 TheCharlatan commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1273305607)
I'll test this.

⚠️ 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?