💬 luke-jr commented on pull request "Intro: Never change the prune checkbox after the user has touched it":
(https://github.com/bitcoin-core/gui/pull/658#discussion_r1247255336)
Yes, removed it
(https://github.com/bitcoin-core/gui/pull/658#discussion_r1247255336)
Yes, removed it
💬 achow101 commented on pull request "net: do not `break` when `addr` is not from a distinct network group":
(https://github.com/bitcoin/bitcoin/pull/27863#issuecomment-1613921442)
ACK 5fa4055452861ca1700008e1761815e88b29fae7
Agree that `continue` makes sense if we're simply unlucky.
(https://github.com/bitcoin/bitcoin/pull/27863#issuecomment-1613921442)
ACK 5fa4055452861ca1700008e1761815e88b29fae7
Agree that `continue` makes sense if we're simply unlucky.
🚀 achow101 merged a pull request: "net: do not `break` when `addr` is not from a distinct network group"
(https://github.com/bitcoin/bitcoin/pull/27863)
(https://github.com/bitcoin/bitcoin/pull/27863)
💬 techy2 commented on pull request "fix: delay in TimeOffset applied to AdjustedTime caused by send/recei…":
(https://github.com/bitcoin/bitcoin/pull/28007#issuecomment-1613931101)
Pointer offset error not flagged in my build. Will close this pull and open a new one with issue resolved.
Testing solution now.
(https://github.com/bitcoin/bitcoin/pull/28007#issuecomment-1613931101)
Pointer offset error not flagged in my build. Will close this pull and open a new one with issue resolved.
Testing solution now.
✅ techy2 closed a pull request: "fix: delay in TimeOffset applied to AdjustedTime caused by send/recei…"
(https://github.com/bitcoin/bitcoin/pull/28007)
(https://github.com/bitcoin/bitcoin/pull/28007)
💬 luke-jr commented on pull request "blockstorage: do not flush block to disk if it is already there":
(https://github.com/bitcoin/bitcoin/pull/27039#discussion_r1247267535)
Won't this result in the undo files not getting flushed when expected?
(https://github.com/bitcoin/bitcoin/pull/27039#discussion_r1247267535)
Won't this result in the undo files not getting flushed when expected?
💬 mzumsande commented on pull request "blockstorage: do not flush block to disk if it is already there":
(https://github.com/bitcoin/bitcoin/pull/27039#discussion_r1247276703)
See https://github.com/bitcoin/bitcoin/pull/27039#pullrequestreview-1320217718 for my thoughts on this, a similar explanation was added to the OP.
(https://github.com/bitcoin/bitcoin/pull/27039#discussion_r1247276703)
See https://github.com/bitcoin/bitcoin/pull/27039#pullrequestreview-1320217718 for my thoughts on this, a similar explanation was added to the OP.
💬 ryanofsky commented on pull request "refactor: Move sock from util to common":
(https://github.com/bitcoin/bitcoin/pull/27989#issuecomment-1613966139)
Concept -0. I don't think it's a good thing to remove everything from util that isn't used by the kernel. I think util/ is a good home for general purpose code providing basic data structures and cross platform capabilities, and I think common/ is good home for project-specific code that needs to be shared between bitcoind, bitcoin-cli, and bitcoin-wallet.
It can make sense to move code from util/ to common/ if the code relies on an external dependency, or has global variables, or something e
...
(https://github.com/bitcoin/bitcoin/pull/27989#issuecomment-1613966139)
Concept -0. I don't think it's a good thing to remove everything from util that isn't used by the kernel. I think util/ is a good home for general purpose code providing basic data structures and cross platform capabilities, and I think common/ is good home for project-specific code that needs to be shared between bitcoind, bitcoin-cli, and bitcoin-wallet.
It can make sense to move code from util/ to common/ if the code relies on an external dependency, or has global variables, or something e
...
📝 techy2 opened a pull request: "fix: delay in TimeOffset applied to AdjustedTime caused by send/receive message queues, correct pointer alignment issue"
(https://github.com/bitcoin/bitcoin/pull/28010)
On busy VPS and shared host with limited resources, the time between when a messages is sent to the tcpip send or receive
queue and when it is sent in the case of send queue, or when it is processed (ProcessMessage) can be in excess of 30 seconds.
This delay introduces a skew in AdjustedTime.
For the receive queue, the post processing uses the receive time prior to entering the queue to calculate TimeOffset rather than Now() which currently includes the delay in the queue.
For the sen
...
(https://github.com/bitcoin/bitcoin/pull/28010)
On busy VPS and shared host with limited resources, the time between when a messages is sent to the tcpip send or receive
queue and when it is sent in the case of send queue, or when it is processed (ProcessMessage) can be in excess of 30 seconds.
This delay introduces a skew in AdjustedTime.
For the receive queue, the post processing uses the receive time prior to entering the queue to calculate TimeOffset rather than Now() which currently includes the delay in the queue.
For the sen
...
💬 jonatack commented on pull request "script, test: python typing and linter updates":
(https://github.com/bitcoin/bitcoin/pull/28009#issuecomment-1613981755)
Per lint ci https://cirrus-ci.com/task/4666772440219648
```
++ test/lint/all-lint.py
Success: no issues found in 268 source files
```
Ready for review.
(https://github.com/bitcoin/bitcoin/pull/28009#issuecomment-1613981755)
Per lint ci https://cirrus-ci.com/task/4666772440219648
```
++ test/lint/all-lint.py
Success: no issues found in 268 source files
```
Ready for review.
👋 jonatack's pull request is ready for review: "script, test: python typing and linter updates"
(https://github.com/bitcoin/bitcoin/pull/28009)
(https://github.com/bitcoin/bitcoin/pull/28009)
💬 ryanofsky commented on pull request "index: make startup more efficient":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1247323044)
In commit "index: verify blocks data existence only once" (5ec5a4888743ce7261e0e9cdc077014cd47bfdd6)
Noticed this while I was rebasing #24230, but one side effect of this commit is now `CustomInit` will be called unnecessarily when the index can't start up because there is a pruning violation. This doesn't seem ideal, but it probably not worth worrying about. It would be good to mention the change in the commit description, though.
I think #24230 will restore previous behavior, and not cal
...
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1247323044)
In commit "index: verify blocks data existence only once" (5ec5a4888743ce7261e0e9cdc077014cd47bfdd6)
Noticed this while I was rebasing #24230, but one side effect of this commit is now `CustomInit` will be called unnecessarily when the index can't start up because there is a pruning violation. This doesn't seem ideal, but it probably not worth worrying about. It would be good to mention the change in the commit description, though.
I think #24230 will restore previous behavior, and not cal
...
💬 luke-jr commented on pull request "Fix logging RPC and -debugexclude with 0/none values, add test coverage, improve docs":
(https://github.com/bitcoin/bitcoin/pull/27231#discussion_r1247319876)
```
src/test/logging_tests.cpp:262:29: warning: loop variable 's' of type 'const std::string&' {aka 'const std::__cxx11::basic_string<char>&'} binds to a temporary constructed from type 'const char* const' [-Wrange-loop-construct]
262 | for (const std::string& s : {"", "NONE", "net", "all", "1"}) {
| ^
src/test/logging_tests.cpp:262:29: note: use non-reference type 'const std::string' {aka 'const std::__cxx11::basic_string<char>'} to make the copy expl
...
(https://github.com/bitcoin/bitcoin/pull/27231#discussion_r1247319876)
```
src/test/logging_tests.cpp:262:29: warning: loop variable 's' of type 'const std::string&' {aka 'const std::__cxx11::basic_string<char>&'} binds to a temporary constructed from type 'const char* const' [-Wrange-loop-construct]
262 | for (const std::string& s : {"", "NONE", "net", "all", "1"}) {
| ^
src/test/logging_tests.cpp:262:29: note: use non-reference type 'const std::string' {aka 'const std::__cxx11::basic_string<char>'} to make the copy expl
...
💬 luke-jr commented on pull request "Fix logging RPC and -debugexclude with 0/none values, add test coverage, improve docs":
(https://github.com/bitcoin/bitcoin/pull/27231#discussion_r1247331119)
Kinda ugly to call `node.logging()` repeatedly
(https://github.com/bitcoin/bitcoin/pull/27231#discussion_r1247331119)
Kinda ugly to call `node.logging()` repeatedly
💬 luke-jr commented on pull request "Fix logging RPC and -debugexclude with 0/none values, add test coverage, improve docs":
(https://github.com/bitcoin/bitcoin/pull/27231#discussion_r1247331674)
Why aren't libevent/leveldb being excluded here?
(https://github.com/bitcoin/bitcoin/pull/27231#discussion_r1247331674)
Why aren't libevent/leveldb being excluded here?
💬 jonatack commented on pull request "Fix logging RPC and -debugexclude with 0/none values, add test coverage, improve docs":
(https://github.com/bitcoin/bitcoin/pull/27231#discussion_r1247340446)
Good point, updated.
```diff
# In addition to net and tor, leveldb/libevent/rand are excluded by the test framework.
+ result = node.logging()
for category in ["leveldb", "libevent", "net", "rand", "tor"]:
- assert not node.logging()[category]
+ assert not result[category]
```
(https://github.com/bitcoin/bitcoin/pull/27231#discussion_r1247340446)
Good point, updated.
```diff
# In addition to net and tor, leveldb/libevent/rand are excluded by the test framework.
+ result = node.logging()
for category in ["leveldb", "libevent", "net", "rand", "tor"]:
- assert not node.logging()[category]
+ assert not result[category]
```
💬 jonatack commented on pull request "Fix logging RPC and -debugexclude with 0/none values, add test coverage, improve docs":
(https://github.com/bitcoin/bitcoin/pull/27231#discussion_r1247342599)
Thank you, Luke, updated. Is that with GCC? I don't see the warning with Clang 16.0.6.
```diff
BOOST_FIXTURE_TEST_CASE(logging_IsNoneCategory, LogSetup)
{
- for (const std::string& s : {"none", "0"}) {
+ for (const char* const& s : {"none", "0"}) {
BOOST_CHECK(LogInstance().IsNoneCategory(s));
}
- for (const std::string& s : {"", "NONE", "net", "all", "1"}) {
+ for (const char* const& s : {"", "NONE", "net", "all", "1"}) {
BOOST_CHECK(!LogInstance
...
(https://github.com/bitcoin/bitcoin/pull/27231#discussion_r1247342599)
Thank you, Luke, updated. Is that with GCC? I don't see the warning with Clang 16.0.6.
```diff
BOOST_FIXTURE_TEST_CASE(logging_IsNoneCategory, LogSetup)
{
- for (const std::string& s : {"none", "0"}) {
+ for (const char* const& s : {"none", "0"}) {
BOOST_CHECK(LogInstance().IsNoneCategory(s));
}
- for (const std::string& s : {"", "NONE", "net", "all", "1"}) {
+ for (const char* const& s : {"", "NONE", "net", "all", "1"}) {
BOOST_CHECK(!LogInstance
...
💬 jonatack commented on pull request "Fix logging RPC and -debugexclude with 0/none values, add test coverage, improve docs":
(https://github.com/bitcoin/bitcoin/pull/27231#discussion_r1247357530)
Because `-debugexclude=none"` is passed. It looks like the passed extra args are concatenated to `self.args` before starting the process, i.e. `self.process = subprocess.Popen(self.args + extra_args...` in `test_framework/test_node.py#L219`, so the "none" value cancels the -debugexclude={libevent,leveldb,rand} args in the same file.
(https://github.com/bitcoin/bitcoin/pull/27231#discussion_r1247357530)
Because `-debugexclude=none"` is passed. It looks like the passed extra args are concatenated to `self.args` before starting the process, i.e. `self.process = subprocess.Popen(self.args + extra_args...` in `test_framework/test_node.py#L219`, so the "none" value cancels the -debugexclude={libevent,leveldb,rand} args in the same file.
💬 jonatack commented on pull request "Fix logging RPC and -debugexclude with 0/none values, add test coverage, improve docs":
(https://github.com/bitcoin/bitcoin/pull/27231#discussion_r1247357713)
Good question, though. I'll add a comment.
(https://github.com/bitcoin/bitcoin/pull/27231#discussion_r1247357713)
Good question, though. I'll add a comment.
💬 MarcoFalke commented on pull request "refactor: Open file in FileCommit":
(https://github.com/bitcoin/bitcoin/pull/28006#issuecomment-1614159773)
Closing for now, until there is a higher need for it.
(https://github.com/bitcoin/bitcoin/pull/28006#issuecomment-1614159773)
Closing for now, until there is a higher need for it.