💬 ariard commented on pull request "policy: restrict all TRUC (v3) transactions to 10KvB":
(https://github.com/bitcoin/bitcoin/pull/29873#issuecomment-2118488640)
I think 10kvb is okay as a limit though pray for someone telling it more to LN folks.
There is very likely 2018 / 2019 channels open with `max_accepted_htlcs` at max 483 both-sides. With no dynamic upgrades deployed, no way to restraint, without force-close and re-open. Worst than pinning exposure is no propagating TRUC transaction due to “naive” upgrade.
In the (far) future, 10kvb could be dynamic at the transaction-relay level, though this would assume LN upgrading its channel policy set
...
(https://github.com/bitcoin/bitcoin/pull/29873#issuecomment-2118488640)
I think 10kvb is okay as a limit though pray for someone telling it more to LN folks.
There is very likely 2018 / 2019 channels open with `max_accepted_htlcs` at max 483 both-sides. With no dynamic upgrades deployed, no way to restraint, without force-close and re-open. Worst than pinning exposure is no propagating TRUC transaction due to “naive” upgrade.
In the (far) future, 10kvb could be dynamic at the transaction-relay level, though this would assume LN upgrading its channel policy set
...
📝 rustyrussell opened a pull request: "doc: note that you can assume C++20."
(https://github.com/bitcoin/bitcoin/pull/30136)
We check this in configure.ac, line 99:
dnl Require C++20 compiler (no GNU extensions)
This was introduced in:
commit fa67f096bdea9db59dd20c470c9e32f3dac5be94
Author: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
Date: Sun Aug 27 10:45:39 2023 +0200
build: Require C++20 compiler
Which git says was before v27.0rc1:
$ git describe --contains fa67f096bdea
v27.0rc1~224^2~4
<!--
*** Please remove the following help text before submitting: ***
Pull requests without
...
(https://github.com/bitcoin/bitcoin/pull/30136)
We check this in configure.ac, line 99:
dnl Require C++20 compiler (no GNU extensions)
This was introduced in:
commit fa67f096bdea9db59dd20c470c9e32f3dac5be94
Author: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
Date: Sun Aug 27 10:45:39 2023 +0200
build: Require C++20 compiler
Which git says was before v27.0rc1:
$ git describe --contains fa67f096bdea
v27.0rc1~224^2~4
<!--
*** Please remove the following help text before submitting: ***
Pull requests without
...
💬 fanquake commented on pull request "util: avoid using thread_local variable that has a destructor":
(https://github.com/bitcoin/bitcoin/pull/30095#discussion_r1605655974)
If we are outlawing thread_local usage, then at the same time, this commit should remove other comments from our codebase suggesting further thread_local usage. i.e [ * If thread-safety is needed, the global could be made thread_local (given](https://github.com/bitcoin/bitcoin/blob/058af75874ffa2b4064e3d6d30cc50f0ec754ba8/src/test/util/random.h#L17)
(https://github.com/bitcoin/bitcoin/pull/30095#discussion_r1605655974)
If we are outlawing thread_local usage, then at the same time, this commit should remove other comments from our codebase suggesting further thread_local usage. i.e [ * If thread-safety is needed, the global could be made thread_local (given](https://github.com/bitcoin/bitcoin/blob/058af75874ffa2b4064e3d6d30cc50f0ec754ba8/src/test/util/random.h#L17)
💬 laanwj commented on pull request "doc: note that you can assume C++20.":
(https://github.com/bitcoin/bitcoin/pull/30136#discussion_r1605657935)
It's useful to document what version of the C++ standard can be used, but i don't think this needs to mention the version it was introduced (the document always applies to the branch it is in), nor is this really the place for documenting a compiler requirement.
Would word it as "Code needs to adhere to the C++20 standard.".
(https://github.com/bitcoin/bitcoin/pull/30136#discussion_r1605657935)
It's useful to document what version of the C++ standard can be used, but i don't think this needs to mention the version it was introduced (the document always applies to the branch it is in), nor is this really the place for documenting a compiler requirement.
Would word it as "Code needs to adhere to the C++20 standard.".
📝 fanquake opened a pull request: "build: Remove `--enable-threadlocal`"
(https://github.com/bitcoin/bitcoin/pull/30137)
Based on #30095 and #30099.
Would close #29952.
Up for debate.
(https://github.com/bitcoin/bitcoin/pull/30137)
Based on #30095 and #30099.
Would close #29952.
Up for debate.
💬 fanquake commented on pull request "build: Enable `thread_local` for MinGW-w64 builds":
(https://github.com/bitcoin/bitcoin/pull/30099#issuecomment-2118613875)
> After this and https://github.com/bitcoin/bitcoin/pull/30095, is there any need to keep the thread_local option at all?
Maybe not. See #30137.
(https://github.com/bitcoin/bitcoin/pull/30099#issuecomment-2118613875)
> After this and https://github.com/bitcoin/bitcoin/pull/30095, is there any need to keep the thread_local option at all?
Maybe not. See #30137.
💬 laanwj commented on pull request "build: Remove `--enable-threadlocal`":
(https://github.com/bitcoin/bitcoin/pull/30137#issuecomment-2118621410)
Concept ACK.
(https://github.com/bitcoin/bitcoin/pull/30137#issuecomment-2118621410)
Concept ACK.
💬 laanwj commented on pull request "wallet, tests: Avoid stringop-overflow warning in PollutePubKey":
(https://github.com/bitcoin/bitcoin/pull/30131#discussion_r1605667276)
Maybe directly assert `pubkey.size() >= 1` to clarify.
(https://github.com/bitcoin/bitcoin/pull/30131#discussion_r1605667276)
Maybe directly assert `pubkey.size() >= 1` to clarify.
⚠️ Geremia opened an issue: "LevelDB error: Corruption: block checksum mismatch didn't trigger reindex."
(https://github.com/bitcoin/bitcoin/issues/30138)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
For the past 29 hours, it's been stuck trying to sync:
>**Number of blocks left** Unknown. Syncing Headers (843771, 100.0%)…
and
>Connecting to peers…
I'm also pruning (`prune=550`).
### Expected behaviour
It should sync the full chain and reindex if needed, not try to sync what it can't (due to possibly corrupt chainstate DB).
### Steps to reproduce
Open up `bitcoin-qt`. It
...
(https://github.com/bitcoin/bitcoin/issues/30138)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
For the past 29 hours, it's been stuck trying to sync:
>**Number of blocks left** Unknown. Syncing Headers (843771, 100.0%)…
and
>Connecting to peers…
I'm also pruning (`prune=550`).
### Expected behaviour
It should sync the full chain and reindex if needed, not try to sync what it can't (due to possibly corrupt chainstate DB).
### Steps to reproduce
Open up `bitcoin-qt`. It
...
💬 hebasto commented on pull request "build: Remove `--enable-threadlocal`":
(https://github.com/bitcoin/bitcoin/pull/30137#issuecomment-2118669335)
Concept ACK.
(https://github.com/bitcoin/bitcoin/pull/30137#issuecomment-2118669335)
Concept ACK.
💬 tdb3 commented on pull request "lint/contrib/doc updates":
(https://github.com/bitcoin/bitcoin/pull/30084#issuecomment-2118676930)
@jonatack @kevkevinpal, what are your thoughts on reopening PR #30106 and dropping the test runner `insuffient` spelling error from 30084?
Seems like this PR (30084) is scoped to more than just the spelling error and fixing the error could help prevent near term lint issues for open PRs.
Maybe I'm missing something and this was already addressed?
(https://github.com/bitcoin/bitcoin/pull/30084#issuecomment-2118676930)
@jonatack @kevkevinpal, what are your thoughts on reopening PR #30106 and dropping the test runner `insuffient` spelling error from 30084?
Seems like this PR (30084) is scoped to more than just the spelling error and fixing the error could help prevent near term lint issues for open PRs.
Maybe I'm missing something and this was already addressed?
📝 fanquake unlocked a pull request: "lint: fixed typo in test_runner causing linter warning"
(https://github.com/bitcoin/bitcoin/pull/30106)
introduced in https://github.com/bitcoin/bitcoin/commit/357ad110548d726021547d85b5b2bfcf3191d7e3
This typo is causing an error in the linter, fixing this should remove this warning
```
test/functional/test_runner.py:651: insuffient ==> insufficient
^ Warning: codespell identified likely spelling errors. Any false positives? Add them to the list of ignored words in test/lint/spelling.ignore-words.txt
```
[link to cirrus ci warning](https://cirrus-ci.com/task/5926490804584448?logs=lint
...
(https://github.com/bitcoin/bitcoin/pull/30106)
introduced in https://github.com/bitcoin/bitcoin/commit/357ad110548d726021547d85b5b2bfcf3191d7e3
This typo is causing an error in the linter, fixing this should remove this warning
```
test/functional/test_runner.py:651: insuffient ==> insufficient
^ Warning: codespell identified likely spelling errors. Any false positives? Add them to the list of ignored words in test/lint/spelling.ignore-words.txt
```
[link to cirrus ci warning](https://cirrus-ci.com/task/5926490804584448?logs=lint
...
💬 laanwj commented on pull request "util: avoid using thread_local variable that has a destructor":
(https://github.com/bitcoin/bitcoin/pull/30095#discussion_r1605695254)
Agree! i mean in a hypothetical future (say, C++30) when every compiler and OS finally implements thread-local storage correctly including destructors, it could be reconsidered, but i think a "no-thread-local" policy makes sense until then.
TBH in general i don't really like comments that suggest TODOs. It's better to keep track of those things in issues where discussion and context is visible.
(https://github.com/bitcoin/bitcoin/pull/30095#discussion_r1605695254)
Agree! i mean in a hypothetical future (say, C++30) when every compiler and OS finally implements thread-local storage correctly including destructors, it could be reconsidered, but i think a "no-thread-local" policy makes sense until then.
TBH in general i don't really like comments that suggest TODOs. It's better to keep track of those things in issues where discussion and context is visible.
💬 laanwj commented on pull request "util: avoid using thread_local variable that has a destructor":
(https://github.com/bitcoin/bitcoin/pull/30095#issuecomment-2118681020)
Code review ACK d35ba1b3f16071b8fe9b36398ba15352dbf2a54d
(https://github.com/bitcoin/bitcoin/pull/30095#issuecomment-2118681020)
Code review ACK d35ba1b3f16071b8fe9b36398ba15352dbf2a54d
💬 TheCharlatan commented on pull request "kernel: De-globalize fReindex":
(https://github.com/bitcoin/bitcoin/pull/29817#discussion_r1605695740)
I ended up deleting all of this in #30132.
(https://github.com/bitcoin/bitcoin/pull/29817#discussion_r1605695740)
I ended up deleting all of this in #30132.
💬 maflcko commented on pull request "lint/contrib/doc updates":
(https://github.com/bitcoin/bitcoin/pull/30084#discussion_r1605700154)
Instead of claiming that this was suggested to be changed by a linter, it would be better to have a real motivation for the change, as well as a reason why the change is correct.
Looking at this, I don't see why continuing is the right behavior change here.
(https://github.com/bitcoin/bitcoin/pull/30084#discussion_r1605700154)
Instead of claiming that this was suggested to be changed by a linter, it would be better to have a real motivation for the change, as well as a reason why the change is correct.
Looking at this, I don't see why continuing is the right behavior change here.
💬 maflcko commented on pull request "lint/contrib/doc updates":
(https://github.com/bitcoin/bitcoin/pull/30084#discussion_r1605699782)
nit: Shouldn't this go into the section that ends with "...could be outdated" above?
(https://github.com/bitcoin/bitcoin/pull/30084#discussion_r1605699782)
nit: Shouldn't this go into the section that ends with "...could be outdated" above?
💬 maflcko commented on issue "LevelDB error: Corruption: block checksum mismatch didn't trigger reindex.":
(https://github.com/bitcoin/bitcoin/issues/30138#issuecomment-2118691502)
Can you clarify this? The debug log says "Shutdown: done", but you claim that "It connects to many peers", indicating that the program is still running?
If this is the case, it would be a bug, because the program should abort and shutdown when file corruption is detected in the leveldb database.
(https://github.com/bitcoin/bitcoin/issues/30138#issuecomment-2118691502)
Can you clarify this? The debug log says "Shutdown: done", but you claim that "It connects to many peers", indicating that the program is still running?
If this is the case, it would be a bug, because the program should abort and shutdown when file corruption is detected in the leveldb database.
💬 jadijadi commented on pull request "Showing Local Addresses in Node Window":
(https://github.com/bitcoin-core/gui/pull/626#issuecomment-2118694174)
> This PR still have a couple of unaddressed comments: [#626 (comment)](https://github.com/bitcoin-core/gui/pull/626#discussion_r915334998) and [#626 (comment)](https://github.com/bitcoin-core/gui/pull/626#discussion_r1274077350).
>
> @jadijadi Are you still working on this?
Will check and comment soon
(https://github.com/bitcoin-core/gui/pull/626#issuecomment-2118694174)
> This PR still have a couple of unaddressed comments: [#626 (comment)](https://github.com/bitcoin-core/gui/pull/626#discussion_r915334998) and [#626 (comment)](https://github.com/bitcoin-core/gui/pull/626#discussion_r1274077350).
>
> @jadijadi Are you still working on this?
Will check and comment soon