💬 hebasto commented on pull request "cmake: Check `-Wno-*` compiler options for `leveldb` target":
(https://github.com/bitcoin/bitcoin/pull/31366#issuecomment-2498964255)
> A more helpful commit message would've been useful though: "Check for -Wfoo rather than -Wno-foo because the latter may not cause the test to fail".

Thanks! The commit message has been amended per your feedback.

💬 theuni commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1857311224)
@hebasto No need to close this if we can change the approach. I think the changes are useful (once the actual bugs are fixed), but your suggestion that other real bugs might have been previously masked set off my alarm bells. Looking through the code now (bench/tests excluded), I don't see any other examples of this.

I think we can agree on an approach where we (in this order):
- Fix the actual bugs
- Notate the false-positives
- Enable the checks.

It sounds like it also is worth adding
...

🤔 glozow reviewed a pull request: "Package validation: accept packages of size 1"
(https://github.com/bitcoin/bitcoin/pull/31096#pullrequestreview-2459618694)
ACK 32fc59796f74a2941772b5ec2755b1319132cd9c

💬 glozow commented on pull request "Package validation: accept packages of size 1":
(https://github.com/bitcoin/bitcoin/pull/31096#discussion_r1857340325)
nit: in the future, best to call `CheckPackageMempoolAcceptResult` beforehand because the errors would be more helpful if they happened (e.g. "error this value should exist" instead of bad optional access)

⚠️ dpc opened an issue: "0.28 binds to default ports when -port or -rpcport set"
(https://github.com/bitcoin/bitcoin/issues/31372)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

```
bitcoind --datadir=(mktemp -d) -port=51234 -rpcport=51235
```

fails for me with:

```
2024-11-25T21:18:29Z [net:error] Unable to bind to 127.0.0.1:8334 on this computer. Bitcoin Core is probably already running.
2024-11-25T21:18:29Z [error] Unable to bind to 127.0.0.1:8334 on this computer. Bitcoin Core is probably already running.
Error: Unable to bind to 127.0.0.1:8334 on t
...

📝 fanquake locked a pull request: "Update README.md"
(https://github.com/bitcoin/bitcoin/pull/31369)
<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that improv
...

📝 fanquake locked a pull request: "Update developer-notes.md"
(https://github.com/bitcoin/bitcoin/pull/31370)
<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that improv
...

💬 mzumsande commented on issue "0.28 binds to default ports when -port or -rpcport set":
(https://github.com/bitcoin/bitcoin/issues/31372#issuecomment-2499089563)
Thanks for the report - this is known, see #31133 . An immediate fix is to use `-bind` instead of `-port`. It is still being discussed whether and how this should be fixed, see the linked issue and https://github.com/bitcoin/bitcoin/pull/31223.

💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1852512769)
nit: `from/by` naming inconsistency, I think my preference would lie with `from` (i.e. update to `kernel_get_block_index_from_hash` and `kernel_get_block_index_from_height`)

(_technically_, could update `kernel_get_next_block_index` -> `kernel_get_block_index_from_previous` and `kernel_get_previous_block_index` -> `kernel_get_block_index_from_next`, but... `from_next` sounds weird?)

💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1850657572)
nit: according to the `worker_threads_num`, 0 is accepted too:
> Zero means no parallel verification.

```suggestion
* used for validation. The number must not be negative. When set to zero, no parallel verification is done.
```

💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1857036990)
I think it's confusing that this function can return `False` and have `status == kernel_SCRIPT_VERIFY_OK`. How about adding a `kernel_SCRIPT_VERIFY_ERROR` catch-all member for unspecified errors? Or alternatively, requiring the user to provide a nullptr and only setting it to `kernel_SCRIPT_VERIFY_OK` is that's actually so?

💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1852510169)
nit: for the other `block_index` getters, `chainstate_manager` is the second argument - would keep that consistent

✅ dpc closed an issue: "0.28 binds to default ports when -port or -rpcport set"
(https://github.com/bitcoin/bitcoin/issues/31372)

💬 dpc commented on issue "0.28 binds to default ports when -port or -rpcport set":
(https://github.com/bitcoin/bitcoin/issues/31372#issuecomment-2499093566)
Thanks. Closing, since a dup.

💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1857397944)
We discussed during the last workshop that ideally we don't have any status codes here at all. But the problem is annoying to tackle. You'd probably want to pass this function a script verify object that has already passed through the required pre-checks. But then you have to either copy the objects into this object, or give ownership up to that object, which I don't think is desirable. The alternative to that is having a function with the same signature that you can call to check the arguments.
...

💬 stickies-v commented on pull request "refactor: Clamp worker threads in ChainstateManager constructor":
(https://github.com/bitcoin/bitcoin/pull/31313#discussion_r1857423512)
Ah, I hadn't considered that. I suppose there's also no real harm in default values being different when accessed from different contexts.
Can be marked as resolved.

💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2499171169)
Thanks for the review @stickies-v!

Updated 34a8429ff3a870c0caaf4c4790becd86c5acde38 -> 35f8503285c672e8ee7e98617e236b38d8ce7a7f ([kernelApi_5](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_5) -> [kernelApi_6](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_6), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_5..kernelApi_6))

* Addressed @stickies-v's [comment](https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1850657572), fixed worker threads docs
...

💬 theStack commented on pull request "ci: limit max stack size to 512 KiB":
(https://github.com/bitcoin/bitcoin/pull/31367#issuecomment-2499192517)
Concept ACK

💬 0xB10C commented on issue "Avoid internet traffic from tests":
(https://github.com/bitcoin/bitcoin/issues/31339#issuecomment-2499241767)
Did you consider running the docker container in CI with `--network none`?

https://docs.docker.com/engine/network/drivers/none/

💬 0xB10C commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-2499335672)
Ran this on my CI runner which has 8.8.8.8 configured as DNS server for docker.

https://cirrus-ci.com/task/5500763260059648?logs=ci#L1137

```
[00:46:26.215] + tcpdump -n -r /tmp/tcpdump_eth0 tcp or udp
[00:46:26.219] 00:42:50.052764 IP 172.18.0.2.46566 > 8.8.8.8.53: 39301+ A? x9.dummySeed.invalid. (38)
[00:46:26.219] 00:42:50.053181 IP 172.18.0.2.58686 > 8.8.8.8.53: 36487+ AAAA? x9.dummySeed.invalid. (38)
[00:46:26.219] 00:42:50.059038 IP 8.8.8.8.53 > 172.18.0.2.46566: 39301 NXDomain 0
...