💬 achow101 commented on pull request "Wallet: don't underestimate the fees when spending a Taproot output":
(https://github.com/bitcoin/bitcoin/pull/26573#issuecomment-2057879299)
ACK 52cc58ab3ad0ee9df9ccc5303c7f0ce85cf8afee modulo rebase
(https://github.com/bitcoin/bitcoin/pull/26573#issuecomment-2057879299)
ACK 52cc58ab3ad0ee9df9ccc5303c7f0ce85cf8afee modulo rebase
💬 achow101 commented on issue "utils: wallet_dump can create a `database` directory, cross-pollinating records":
(https://github.com/bitcoin/bitcoin/issues/29883#issuecomment-2057925564)
Hmm, the contents of the log files shouldn't matter since the database files have their LSNs reset when the wallet is closed. 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?
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.
(https://github.com/bitcoin/bitcoin/issues/29883#issuecomment-2057925564)
Hmm, the contents of the log files shouldn't matter since the database files have their LSNs reset when the wallet is closed. 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?
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.
💬 andrewtoth commented on pull request "index: race fix, lock cs_main while 'm_synced' is subject to change":
(https://github.com/bitcoin/bitcoin/pull/29867#issuecomment-2057954213)
> > Is this related to [bbe82c1](https://github.com/bitcoin/bitcoin/commit/bbe82c116e72ca0638751e063bf564cd1fe5c4d5)?
>
> No, it is related to [0faafb5](https://github.com/bitcoin/bitcoin/commit/0faafb57f8298547949cbc0044ee9e925ed887ba). This PR partially reverts it and documents the flow so the regression does not happen again.
Is it worth it to partially revert? Why not just `git revert 0faafb57f8298547949cbc0044ee9e925ed887ba`? In the happy path it is only locking for a check if there i
...
(https://github.com/bitcoin/bitcoin/pull/29867#issuecomment-2057954213)
> > Is this related to [bbe82c1](https://github.com/bitcoin/bitcoin/commit/bbe82c116e72ca0638751e063bf564cd1fe5c4d5)?
>
> No, it is related to [0faafb5](https://github.com/bitcoin/bitcoin/commit/0faafb57f8298547949cbc0044ee9e925ed887ba). This PR partially reverts it and documents the flow so the regression does not happen again.
Is it worth it to partially revert? Why not just `git revert 0faafb57f8298547949cbc0044ee9e925ed887ba`? In the happy path it is only locking for a check if there i
...
💬 pablomartin4btc commented on pull request "assumeutxo, rpc: Improve EOF error when reading snapshot metadata in loadtxoutset":
(https://github.com/bitcoin/bitcoin/pull/28670#issuecomment-2058025128)
Updates:
- I think [lint CI failure](https://github.com/bitcoin/bitcoin/pull/28670/checks?check_run_id=20626357772) is unrelated with the code change, it was running successful before started failing with no code change, run `./test/lint/lint-python.py` locally with no failure.
(https://github.com/bitcoin/bitcoin/pull/28670#issuecomment-2058025128)
Updates:
- I think [lint CI failure](https://github.com/bitcoin/bitcoin/pull/28670/checks?check_run_id=20626357772) is unrelated with the code change, it was running successful before started failing with no code change, run `./test/lint/lint-python.py` locally with no failure.
🤔 furszy reviewed a pull request: "assumeutxo, rpc: Improve EOF error when reading snapshot metadata in loadtxoutset"
(https://github.com/bitcoin/bitcoin/pull/28670#pullrequestreview-2002419037)
The CI failure is due a merge conflict. Need to rebase the PR. The code you are using is not there anymore in master.
(https://github.com/bitcoin/bitcoin/pull/28670#pullrequestreview-2002419037)
The CI failure is due a merge conflict. Need to rebase the PR. The code you are using is not there anymore in master.
💬 ariard commented on pull request "policy: restrict all TRUC (v3) transactions to 25KvB":
(https://github.com/bitcoin/bitcoin/pull/29873#issuecomment-2058091748)
> [Expected size of a commitment transaction](https://github.com/lightning/bolts/blob/master/03-transactions.md#expected-weight-of-the-commitment-transaction) is within (900 + 172 * 483 + 224) / 4 = 21050vB
Check BOLT2’s `max_accepted_htlcs`, it’s 483 HTLC outputs in both directions (inbounds / outbounds), so 25k virtual bytes doesn’t work (even if LN implems use more conservative limits in practice, they’re exposed to lightning node operators).
> Would this make things {broken, inconvenie
...
(https://github.com/bitcoin/bitcoin/pull/29873#issuecomment-2058091748)
> [Expected size of a commitment transaction](https://github.com/lightning/bolts/blob/master/03-transactions.md#expected-weight-of-the-commitment-transaction) is within (900 + 172 * 483 + 224) / 4 = 21050vB
Check BOLT2’s `max_accepted_htlcs`, it’s 483 HTLC outputs in both directions (inbounds / outbounds), so 25k virtual bytes doesn’t work (even if LN implems use more conservative limits in practice, they’re exposed to lightning node operators).
> Would this make things {broken, inconvenie
...
💬 sipa commented on pull request "fuzz: explicitly cap the vsize of RBFs for diagram checks":
(https://github.com/bitcoin/bitcoin/pull/29879#discussion_r1566641643)
Is there a need to abort the fuzz test when this happens (here and below), or could this be a `break` to allow the test to proceed still, without the transaction that would make the total overflow?
(https://github.com/bitcoin/bitcoin/pull/29879#discussion_r1566641643)
Is there a need to abort the fuzz test when this happens (here and below), or could this be a `break` to allow the test to proceed still, without the transaction that would make the total overflow?
💬 pablomartin4btc commented on pull request "assumeutxo, rpc: Improve EOF error when reading snapshot metadata in loadtxoutset":
(https://github.com/bitcoin/bitcoin/pull/28670#issuecomment-2058142382)
Rebased.
(https://github.com/bitcoin/bitcoin/pull/28670#issuecomment-2058142382)
Rebased.
🤔 tdb3 reviewed a pull request: "netbase: clean up Proxy logging"
(https://github.com/bitcoin/bitcoin/pull/29882#pullrequestreview-2002495493)
ACK for d1af4422d13ba84253a1555c2fe5a42170fa8b35
Good cleanup, making logs less cluttered while keeping relevant connection state logging.
In addition to observing the lack of `Cannot connect to socket for` message, also ran a quick test where Tor was stopped then started again. bitcoind was still able to detect and inform the user of the loss of the proxy and subsequent `peer connected` message provided indication that connection through proxy was reestablished.
```
13308 tx=3456746 da
...
(https://github.com/bitcoin/bitcoin/pull/29882#pullrequestreview-2002495493)
ACK for d1af4422d13ba84253a1555c2fe5a42170fa8b35
Good cleanup, making logs less cluttered while keeping relevant connection state logging.
In addition to observing the lack of `Cannot connect to socket for` message, also ran a quick test where Tor was stopped then started again. bitcoind was still able to detect and inform the user of the loss of the proxy and subsequent `peer connected` message provided indication that connection through proxy was reestablished.
```
13308 tx=3456746 da
...
💬 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
```