💬 tdb3 commented on pull request "netbase: clean up Proxy logging":
(https://github.com/bitcoin/bitcoin/pull/29882#discussion_r1566647446)
I could be mistaken, but it looks at first glance that this would be covered in `ConnectToSocket()`:
https://github.com/bitcoin/bitcoin/blob/d1af4422d13ba84253a1555c2fe5a42170fa8b35/src/netbase.cpp#L603
and reached in Proxy::Connect() through either the call to `ConnectToSocket()`:
https://github.com/bitcoin/bitcoin/blob/d1af4422d13ba84253a1555c2fe5a42170fa8b35/src/netbase.cpp#L653
or by way of `ConnectDirectly()`:
https://github.com/bitcoin/bitcoin/blob/d1af4422d13ba84253a1555c2fe5a4
...
(https://github.com/bitcoin/bitcoin/pull/29882#discussion_r1566647446)
I could be mistaken, but it looks at first glance that this would be covered in `ConnectToSocket()`:
https://github.com/bitcoin/bitcoin/blob/d1af4422d13ba84253a1555c2fe5a42170fa8b35/src/netbase.cpp#L603
and reached in Proxy::Connect() through either the call to `ConnectToSocket()`:
https://github.com/bitcoin/bitcoin/blob/d1af4422d13ba84253a1555c2fe5a42170fa8b35/src/netbase.cpp#L653
or by way of `ConnectDirectly()`:
https://github.com/bitcoin/bitcoin/blob/d1af4422d13ba84253a1555c2fe5a4
...
💬 laanwj commented on issue "utils: wallet_dump can create a `database` directory, cross-pollinating records":
(https://github.com/bitcoin/bitcoin/issues/29883#issuecomment-2058216588)
> Is it possible that the wallet files were last opened in an unclean shutdown, or maybe with older versions that didn't always flush on shutdown?
Yes, this is possible, it's a random grab bag of wallet database files (not even sure there's not a litecoin or dogecoin one in there).
> Also, yes, having duplicate files will result in the same unique id which means that the log files can be applied to the wrong file, which will really mess things up.
Honestly after this train wreck i'm at
...
(https://github.com/bitcoin/bitcoin/issues/29883#issuecomment-2058216588)
> Is it possible that the wallet files were last opened in an unclean shutdown, or maybe with older versions that didn't always flush on shutdown?
Yes, this is possible, it's a random grab bag of wallet database files (not even sure there's not a litecoin or dogecoin one in there).
> Also, yes, having duplicate files will result in the same unique id which means that the log files can be applied to the wrong file, which will really mess things up.
Honestly after this train wreck i'm at
...
💬 laanwj commented on pull request "netbase: clean up Proxy logging":
(https://github.com/bitcoin/bitcoin/pull/29882#discussion_r1566708786)
Yes, it's not strictly currently necessary, but code changes sometimes. It's good to have error and unexpected situation checking in place, instead of crashing later on.
(https://github.com/bitcoin/bitcoin/pull/29882#discussion_r1566708786)
Yes, it's not strictly currently necessary, but code changes sometimes. It's good to have error and unexpected situation checking in place, instead of crashing later on.
📝 OkSang88 opened a pull request: "BTC world"
(https://github.com/bitcoin/bitcoin/pull/29885)
`<!--`
*** 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 impr
...
(https://github.com/bitcoin/bitcoin/pull/29885)
`<!--`
*** 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 impr
...
✅ OkSang88 closed a pull request: "BTC world"
(https://github.com/bitcoin/bitcoin/pull/29885)
(https://github.com/bitcoin/bitcoin/pull/29885)
📝 OkSang88 reopened a pull request: "BTC world"
(https://github.com/bitcoin/bitcoin/pull/29885)
`<!--`
*** 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 impr
...
(https://github.com/bitcoin/bitcoin/pull/29885)
`<!--`
*** 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 impr
...
✅ laanwj closed a pull request: "BTC team"
(https://github.com/bitcoin/bitcoin/pull/29885)
(https://github.com/bitcoin/bitcoin/pull/29885)
👋 fanquake's pull request is ready for review: "build: add `-Wundef`"
(https://github.com/bitcoin/bitcoin/pull/29876)
(https://github.com/bitcoin/bitcoin/pull/29876)
📝 fanquake locked a pull request: "(deleted)"
(https://github.com/bitcoin/bitcoin/pull/29885)
(https://github.com/bitcoin/bitcoin/pull/29885)
💬 laanwj commented on pull request "refactor/test: add a few more base32/64 calculation corner cases":
(https://github.com/bitcoin/bitcoin/pull/29847#issuecomment-2058276413)
Thanks for your first contribution!
As it is hard to review PRs that change things all over the place, this is unlikely to be merged this way. I'd suggest keeping the code change focused. For example, make it add test cases only, and remove the changes (refactoring and otherwise) to `strecodings.cpp` and other non-test code.
(https://github.com/bitcoin/bitcoin/pull/29847#issuecomment-2058276413)
Thanks for your first contribution!
As it is hard to review PRs that change things all over the place, this is unlikely to be merged this way. I'd suggest keeping the code change focused. For example, make it add test cases only, and remove the changes (refactoring and otherwise) to `strecodings.cpp` and other non-test code.
💬 laanwj commented on issue "utils: wallet_dump can create a `database` directory, cross-pollinating records":
(https://github.com/bitcoin/bitcoin/issues/29883#issuecomment-2058298297)
You wee right: with the LSN check, one of the wallets from 2015 trips up:
```
terminate called after throwing an instance of 'std::runtime_error'
what(): LSNs are not reset, this database is not completely flushed. Please reopen then close the database with a version that has BDB support
```
(https://github.com/bitcoin/bitcoin/issues/29883#issuecomment-2058298297)
You wee right: with the LSN check, one of the wallets from 2015 trips up:
```
terminate called after throwing an instance of 'std::runtime_error'
what(): LSNs are not reset, this database is not completely flushed. Please reopen then close the database with a version that has BDB support
```
💬 laanwj commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#issuecomment-2058305830)
> There was a comment at CoreDev that this should check that the file is compacted, so I've added a commit that does that.
That's good to have. It does look like the newly added check trips up in the CI in some places:
```
unknown location(0): fatal error: in "db_tests/db_cursor_prefix_range_test": class std::runtime_error: LSNs are not reset, this database is not completely flushed. Please reopen then close the database with a version that has BDB support
```
(https://github.com/bitcoin/bitcoin/pull/26606#issuecomment-2058305830)
> There was a comment at CoreDev that this should check that the file is compacted, so I've added a commit that does that.
That's good to have. It does look like the newly added check trips up in the CI in some places:
```
unknown location(0): fatal error: in "db_tests/db_cursor_prefix_range_test": class std::runtime_error: LSNs are not reset, this database is not completely flushed. Please reopen then close the database with a version that has BDB support
```
💬 laanwj commented on pull request "rpc: return warnings as an array instead of just a single one":
(https://github.com/bitcoin/bitcoin/pull/29845#issuecomment-2058329366)
This is something that we should have done a long time ago imo. I wouldn't be surprised if I have an ancient PR doing just this.
Concept ACK!
(https://github.com/bitcoin/bitcoin/pull/29845#issuecomment-2058329366)
This is something that we should have done a long time ago imo. I wouldn't be surprised if I have an ancient PR doing just this.
Concept ACK!
💬 paplorinc commented on pull request "refactor/test: add a few more base32/64 calculation corner cases":
(https://github.com/bitcoin/bitcoin/pull/29847#issuecomment-2058381680)
Hey @laanwj, thanks for your first review.
Please see the commits separately, don't worry, they're not all over the place.
(https://github.com/bitcoin/bitcoin/pull/29847#issuecomment-2058381680)
Hey @laanwj, thanks for your first review.
Please see the commits separately, don't worry, they're not all over the place.
💬 maflcko commented on pull request "refactor/test: add a few more base32/64 calculation corner cases":
(https://github.com/bitcoin/bitcoin/pull/29847#discussion_r1566832562)
Why this change? This style change makes the code harder to read.
Unless there is a reason to change the code and review it, it shouldn't be changed.
(https://github.com/bitcoin/bitcoin/pull/29847#discussion_r1566832562)
Why this change? This style change makes the code harder to read.
Unless there is a reason to change the code and review it, it shouldn't be changed.
💬 maflcko commented on pull request "refactor/test: add a few more base32/64 calculation corner cases":
(https://github.com/bitcoin/bitcoin/pull/29847#issuecomment-2058390006)
@paplorinc If you are not being receptive to reviewers' feedback, it is unlikely that anyone will review the pull request and without review it will not be merged.
(https://github.com/bitcoin/bitcoin/pull/29847#issuecomment-2058390006)
@paplorinc If you are not being receptive to reviewers' feedback, it is unlikely that anyone will review the pull request and without review it will not be merged.
💬 paplorinc commented on pull request "refactor/test: add a few more base32/64 calculation corner cases":
(https://github.com/bitcoin/bitcoin/pull/29847#discussion_r1566838275)
I've changed it to unify it with the previous lambda. Why would I leave the type there when I've used auto 2 lines above?
(https://github.com/bitcoin/bitcoin/pull/29847#discussion_r1566838275)
I've changed it to unify it with the previous lambda. Why would I leave the type there when I've used auto 2 lines above?
💬 paplorinc commented on pull request "refactor/test: add a few more base32/64 calculation corner cases":
(https://github.com/bitcoin/bitcoin/pull/29847#issuecomment-2058398857)
@maflcko, I've responded to every single comment within minutes. I just don't appreciate the patronizing style I keep seeing in this repo.
(https://github.com/bitcoin/bitcoin/pull/29847#issuecomment-2058398857)
@maflcko, I've responded to every single comment within minutes. I just don't appreciate the patronizing style I keep seeing in this repo.
🤔 fjahr reviewed a pull request: "assumeutxo, rpc: Improve EOF error when reading snapshot metadata in loadtxoutset"
(https://github.com/bitcoin/bitcoin/pull/28670#pullrequestreview-2002828900)
re-ACK 521de52c751a4539333bb2f69a07c0f1b4f496de
(https://github.com/bitcoin/bitcoin/pull/28670#pullrequestreview-2002828900)
re-ACK 521de52c751a4539333bb2f69a07c0f1b4f496de
💬 fjahr commented on pull request "assumeutxo, rpc: Improve EOF error when reading snapshot metadata in loadtxoutset":
(https://github.com/bitcoin/bitcoin/pull/28670#discussion_r1566860922)
nit: I think this info log could be improved if you retouch. Testing the file size is not really the goal, it's about hitting the metadata parsing error that was added, right? So I would suggest to call it something like "Inablity to parse metadata fails early" or so.
(https://github.com/bitcoin/bitcoin/pull/28670#discussion_r1566860922)
nit: I think this info log could be improved if you retouch. Testing the file size is not really the goal, it's about hitting the metadata parsing error that was added, right? So I would suggest to call it something like "Inablity to parse metadata fails early" or so.