✅ fanquake closed a pull request: "Update COPYING"
(https://github.com/bitcoin/bitcoin/pull/31368)
(https://github.com/bitcoin/bitcoin/pull/31368)
📝 fanquake locked a pull request: "Update COPYING"
(https://github.com/bitcoin/bitcoin/pull/31368)
<!--
*** 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
...
(https://github.com/bitcoin/bitcoin/pull/31368)
<!--
*** 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
...
💬 theuni commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1857286619)
> A bare NOLINT, without any explanation, has never meant "I'm smarter than my static analyzer"—at least, not to me.
I'm not sure what else it could mean.
[From the docs](https://clang.llvm.org/extra/clang-tidy/): "clang-tidy diagnostics are intended to call out code that does not adhere to a coding standard, or is otherwise problematic in some way. However, if the code is known to be correct, it may be useful to silence the warning"
Does this mean there are other places in the code whe
...
(https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1857286619)
> A bare NOLINT, without any explanation, has never meant "I'm smarter than my static analyzer"—at least, not to me.
I'm not sure what else it could mean.
[From the docs](https://clang.llvm.org/extra/clang-tidy/): "clang-tidy diagnostics are intended to call out code that does not adhere to a coding standard, or is otherwise problematic in some way. However, if the code is known to be correct, it may be useful to silence the warning"
Does this mean there are other places in the code whe
...
📝 ijunxyz123 opened 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
...
(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
...
👍 theuni approved a pull request: "refactor: Fix remaining clang-tidy performance-unnecessary-copy-initialization errors"
(https://github.com/bitcoin/bitcoin/pull/31364#pullrequestreview-2459538939)
ACK the much more constrained 3305972f7bfd78181566e4297891c2dd7cae0f2b.
(https://github.com/bitcoin/bitcoin/pull/31364#pullrequestreview-2459538939)
ACK the much more constrained 3305972f7bfd78181566e4297891c2dd7cae0f2b.
📝 ijunxyz123 opened 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
...
(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
...
💬 hebasto commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1857290156)
> I'm not sure what else it could mean.
>
> [From the docs](https://clang.llvm.org/extra/clang-tidy/): "clang-tidy diagnostics are intended to call out code that does not adhere to a coding standard, or is otherwise problematic in some way. However, if the code is known to be correct, it may be useful to silence the warning"
You are right.
(https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1857290156)
> I'm not sure what else it could mean.
>
> [From the docs](https://clang.llvm.org/extra/clang-tidy/): "clang-tidy diagnostics are intended to call out code that does not adhere to a coding standard, or is otherwise problematic in some way. However, if the code is known to be correct, it may be useful to silence the warning"
You are right.
✅ willcl-ark closed a pull request: "Update developer-notes.md"
(https://github.com/bitcoin/bitcoin/pull/31370)
(https://github.com/bitcoin/bitcoin/pull/31370)
✅ willcl-ark closed a pull request: "Update README.md"
(https://github.com/bitcoin/bitcoin/pull/31369)
(https://github.com/bitcoin/bitcoin/pull/31369)
💬 willcl-ark commented on pull request "Update README.md":
(https://github.com/bitcoin/bitcoin/pull/31369#issuecomment-2498948670)
Please don't open pull requests like this here, you can use your own fork for testing things out.
(https://github.com/bitcoin/bitcoin/pull/31369#issuecomment-2498948670)
Please don't open pull requests like this here, you can use your own fork for testing things out.
📝 theStack opened a pull request: "doc, test: more ephemeral dust follow-ups"
(https://github.com/bitcoin/bitcoin/pull/31371)
This small PR contains ephemeral dust follow-ups mentioned in #30329 that were not tackled in the first follow-up PR #31279:
https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1828577696
https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1825279952
Happy to add more if I missed some or anyone has concrete commits to add.
(https://github.com/bitcoin/bitcoin/pull/31371)
This small PR contains ephemeral dust follow-ups mentioned in #30329 that were not tackled in the first follow-up PR #31279:
https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1828577696
https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1825279952
Happy to add more if I missed some or anyone has concrete commits to add.
💬 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.
(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
...
(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
(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)
(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
...
(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
...
(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
...
(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.
(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?)
(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?)