💬 maflcko commented on pull request "log: Remove brittle and confusing LogPrintLevel":
(https://github.com/bitcoin/bitcoin/pull/34051#discussion_r2613733349)
Thanks for providing a diff. I've pushed a commit with that:
```diff
$ git diff fa35682637 fa33a9f5be -U0
diff --git a/src/test/logging_tests.cpp b/src/test/logging_tests.cpp
index 3d95c97426..2a34aefae1 100644
--- a/src/test/logging_tests.cpp
+++ b/src/test/logging_tests.cpp
@@ -187,0 +188 @@ BOOST_FIXTURE_TEST_CASE(logging_SeverityLevels, LogSetup)
+ LogInstance().SetLogLevel(BCLog::Level::Debug);
(https://github.com/bitcoin/bitcoin/pull/34051#discussion_r2613733349)
Thanks for providing a diff. I've pushed a commit with that:
```diff
$ git diff fa35682637 fa33a9f5be -U0
diff --git a/src/test/logging_tests.cpp b/src/test/logging_tests.cpp
index 3d95c97426..2a34aefae1 100644
--- a/src/test/logging_tests.cpp
+++ b/src/test/logging_tests.cpp
@@ -187,0 +188 @@ BOOST_FIXTURE_TEST_CASE(logging_SeverityLevels, LogSetup)
+ LogInstance().SetLogLevel(BCLog::Level::Debug);
💬 maflcko commented on issue "Showing error:- Error unlocking wallet: some keys decrypt but not all. Your wallet file may be corrupt.":
(https://github.com/bitcoin/bitcoin/issues/34055#issuecomment-3645935489)
> 1. Does it means mine wallet.dat is corrupt?
>
> 2. Password is wrong ?
>
> 3. bitcoin core is not synced fully ?
>
>
> Please Help me
When it says "Error unlocking wallet: some keys decrypt but not all. Your wallet file may be corrupt.", then it means "Error unlocking wallet: some keys decrypt but not all. Your wallet file may be corrupt.".
You can continue at your own risk, but my recommendation would be to backup your wallet.dat, and also restore an old backup and double che
...
(https://github.com/bitcoin/bitcoin/issues/34055#issuecomment-3645935489)
> 1. Does it means mine wallet.dat is corrupt?
>
> 2. Password is wrong ?
>
> 3. bitcoin core is not synced fully ?
>
>
> Please Help me
When it says "Error unlocking wallet: some keys decrypt but not all. Your wallet file may be corrupt.", then it means "Error unlocking wallet: some keys decrypt but not all. Your wallet file may be corrupt.".
You can continue at your own risk, but my recommendation would be to backup your wallet.dat, and also restore an old backup and double che
...
💬 maflcko commented on issue "Showing error:- Error unlocking wallet: some keys decrypt but not all. Your wallet file may be corrupt.":
(https://github.com/bitcoin/bitcoin/issues/34055#issuecomment-3645936203)
Usually the issue tracker is used to track technical issues related to the Bitcoin Core code base.
General bitcoin questions and/or support requests are best directed to the [Bitcoin StackExchange](https://bitcoin.stackexchange.com) or the `#bitcoin` IRC channel on Libera Chat, or one of the Bitcoin subreddits, or any other place that you feel is well suited.
(https://github.com/bitcoin/bitcoin/issues/34055#issuecomment-3645936203)
Usually the issue tracker is used to track technical issues related to the Bitcoin Core code base.
General bitcoin questions and/or support requests are best directed to the [Bitcoin StackExchange](https://bitcoin.stackexchange.com) or the `#bitcoin` IRC channel on Libera Chat, or one of the Bitcoin subreddits, or any other place that you feel is well suited.
✅ maflcko closed an issue: "Showing error:- Error unlocking wallet: some keys decrypt but not all. Your wallet file may be corrupt."
(https://github.com/bitcoin/bitcoin/issues/34055)
(https://github.com/bitcoin/bitcoin/issues/34055)
✅ fanquake closed a pull request: "p2p: avoid traversing blocks (twice) during IBD"
(https://github.com/bitcoin/bitcoin/pull/32730)
(https://github.com/bitcoin/bitcoin/pull/32730)
🚀 fanquake merged a pull request: "net: Waste less time in socket handling"
(https://github.com/bitcoin/bitcoin/pull/34025)
(https://github.com/bitcoin/bitcoin/pull/34025)
💬 stickies-v commented on pull request "log: Remove brittle and confusing LogPrintLevel":
(https://github.com/bitcoin/bitcoin/pull/34051#issuecomment-3645987726)
re-ACK fa33a9f5be1beabfee6c217d133cc51dea3b9a63
No changes except improved self-documentation of test, ty!
(https://github.com/bitcoin/bitcoin/pull/34051#issuecomment-3645987726)
re-ACK fa33a9f5be1beabfee6c217d133cc51dea3b9a63
No changes except improved self-documentation of test, ty!
👍 rkrux approved a pull request: "log: Remove brittle and confusing LogPrintLevel"
(https://github.com/bitcoin/bitcoin/pull/34051#pullrequestreview-3571354142)
ACK fa33a9f5be1beabfee6c217d133cc51dea3b9a63
This is helpful. Having gone through the logging code recently, I found myself confused a bit upon seeing different ways to log. Removal of `LogPrintLevel` helps in decreasing the confusion by just using `LOG<$LEVEL>()` macros.
(https://github.com/bitcoin/bitcoin/pull/34051#pullrequestreview-3571354142)
ACK fa33a9f5be1beabfee6c217d133cc51dea3b9a63
This is helpful. Having gone through the logging code recently, I found myself confused a bit upon seeing different ways to log. Removal of `LogPrintLevel` helps in decreasing the confusion by just using `LOG<$LEVEL>()` macros.
💬 stickies-v commented on pull request "lint: Remove confusing, redundant, and brittle lint-spelling":
(https://github.com/bitcoin/bitcoin/pull/34053#issuecomment-3645998330)
Concept ACK. Didn't seem to be a useful test, so best to remove it.
(https://github.com/bitcoin/bitcoin/pull/34053#issuecomment-3645998330)
Concept ACK. Didn't seem to be a useful test, so best to remove it.
📝 optout21 converted_to_draft a pull request: "test: Add test on skip heights in CBlockIndex"
(https://github.com/bitcoin/bitcoin/pull/33661)
The skip height values, used by `CBlockIndex::BuildSkip()` and `GetSkipHeight` are not tested and not well documented. (noticed while reviewing #33515, recently merged).
The motivation is to document the skip value computation through a test.
The first commit adds a test for the properties of the distribution of skip values, namely that they have non-uniform distribution: most values are small but there are some large ones as well.
The second commit adds low-level tests to the `GetSkipH
...
(https://github.com/bitcoin/bitcoin/pull/33661)
The skip height values, used by `CBlockIndex::BuildSkip()` and `GetSkipHeight` are not tested and not well documented. (noticed while reviewing #33515, recently merged).
The motivation is to document the skip value computation through a test.
The first commit adds a test for the properties of the distribution of skip values, namely that they have non-uniform distribution: most values are small but there are some large ones as well.
The second commit adds low-level tests to the `GetSkipH
...
⚠️ husstege-e opened an issue: "Wallet RPC semantics: gettransaction.confirmations returns large negative values for RBF-replaced transactions (undocumented behavior)"
(https://github.com/bitcoin/bitcoin/issues/34056)
### Is there an existing issue for this?
- [x] I have searched the existing issues
### Current behaviour
When using Replace-By-Fee (RBF) in combination with wallet rescans, the wallet RPC reports transactions with negative confirmation counts (e.g. -900, -1200), long after the replacing transaction has been confirmed.
These negative confirmation values persist even when:
- the replacement transaction is deeply confirmed
- the original transaction is fully conflicted and no longer in the memp
...
(https://github.com/bitcoin/bitcoin/issues/34056)
### Is there an existing issue for this?
- [x] I have searched the existing issues
### Current behaviour
When using Replace-By-Fee (RBF) in combination with wallet rescans, the wallet RPC reports transactions with negative confirmation counts (e.g. -900, -1200), long after the replacing transaction has been confirmed.
These negative confirmation values persist even when:
- the replacement transaction is deeply confirmed
- the original transaction is fully conflicted and no longer in the memp
...
👍 rkrux approved a pull request: "lint: Remove confusing, redundant, and brittle lint-spelling"
(https://github.com/bitcoin/bitcoin/pull/34053#pullrequestreview-3571429071)
ACK fa904fc683c0892eb800ddb986fdc0c646721077
(https://github.com/bitcoin/bitcoin/pull/34053#pullrequestreview-3571429071)
ACK fa904fc683c0892eb800ddb986fdc0c646721077
💬 Ataraxia009 commented on pull request "log: Remove brittle and confusing LogPrintLevel":
(https://github.com/bitcoin/bitcoin/pull/34051#discussion_r2613872871)
I still don't see the point of leaving the `Assume` in there since it seems to make no difference other than constricting somebody that would want to create a new macro for `LogInfo` w category. But i wont hold the PR back because of it.
(https://github.com/bitcoin/bitcoin/pull/34051#discussion_r2613872871)
I still don't see the point of leaving the `Assume` in there since it seems to make no difference other than constricting somebody that would want to create a new macro for `LogInfo` w category. But i wont hold the PR back because of it.
💬 rkrux commented on pull request "wallet, doc: clarify the coin selection filters that enforce cluster count":
(https://github.com/bitcoin/bitcoin/pull/34037#issuecomment-3646124212)
> Since https://github.com/bitcoin/bitcoin/pull/33639, these filters have started using cluster count instead of max desc count across ancestors.
The linked PR doesn't seem correct to me. Is it supposed to be #33629 instead?
(https://github.com/bitcoin/bitcoin/pull/34037#issuecomment-3646124212)
> Since https://github.com/bitcoin/bitcoin/pull/33639, these filters have started using cluster count instead of max desc count across ancestors.
The linked PR doesn't seem correct to me. Is it supposed to be #33629 instead?
💬 musaHaruna commented on pull request "rpc: Distinguish between vsize and sigop adjusted mempool vsize":
(https://github.com/bitcoin/bitcoin/pull/32800#issuecomment-3646162211)
Rebased and fixed conflict.
(https://github.com/bitcoin/bitcoin/pull/32800#issuecomment-3646162211)
Rebased and fixed conflict.
💬 maflcko commented on pull request "log: Remove brittle and confusing LogPrintLevel":
(https://github.com/bitcoin/bitcoin/pull/34051#discussion_r2613946723)
> I still don't see the point of leaving the `Assume` in there since it seems to make no difference
The point of the `Assume` is to document internal assumptions for developers. The docs say `No rate limiting is applied, `. So writing an `Assume(!rate_limit)` ensures this to some extent for developers and documents it further.
The assume is just a trivial single line, I don't see the risk to keep it. It is also more trivial to remove, than to update the doc string itself.
I'll leave as-
...
(https://github.com/bitcoin/bitcoin/pull/34051#discussion_r2613946723)
> I still don't see the point of leaving the `Assume` in there since it seems to make no difference
The point of the `Assume` is to document internal assumptions for developers. The docs say `No rate limiting is applied, `. So writing an `Assume(!rate_limit)` ensures this to some extent for developers and documents it further.
The assume is just a trivial single line, I don't see the risk to keep it. It is also more trivial to remove, than to update the doc string itself.
I'll leave as-
...
👋 optout21's pull request is ready for review: "test: Add test on skip heights in CBlockIndex"
(https://github.com/bitcoin/bitcoin/pull/33661)
(https://github.com/bitcoin/bitcoin/pull/33661)
✅ maflcko closed an issue: "Wallet RPC semantics: gettransaction.confirmations returns large negative values for RBF-replaced transactions (undocumented behavior)"
(https://github.com/bitcoin/bitcoin/issues/34056)
(https://github.com/bitcoin/bitcoin/issues/34056)
💬 maflcko commented on issue "Wallet RPC semantics: gettransaction.confirmations returns large negative values for RBF-replaced transactions (undocumented behavior)":
(https://github.com/bitcoin/bitcoin/issues/34056#issuecomment-3646182093)
> undocumented
To get the documentation, you can use the `help` RPC. It will explain and document the output. E.g.
```
"confirmations" : n, (numeric) The number of confirmations for the transaction. Negative confirmations means the
transaction conflicted that many blocks ago.
(https://github.com/bitcoin/bitcoin/issues/34056#issuecomment-3646182093)
> undocumented
To get the documentation, you can use the `help` RPC. It will explain and document the output. E.g.
```
"confirmations" : n, (numeric) The number of confirmations for the transaction. Negative confirmations means the
transaction conflicted that many blocks ago.
💬 optout21 commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#issuecomment-3646190934)
I redid the test, following @sipa's remark.
- The test on skip height properties -- median, avg -- is dropped
- A new test is added, that tests the skip heights _indirectly_, by doing a bunch of ancestor searches, and counting the number of steps needed, and doing some asserts on the step count. In a 200k long chain a series 1000 runs are performed. In each run a start and a target heights are picked randomly: the start is in the upper half of the chain, and the target is below it, by at lea
...
(https://github.com/bitcoin/bitcoin/pull/33661#issuecomment-3646190934)
I redid the test, following @sipa's remark.
- The test on skip height properties -- median, avg -- is dropped
- A new test is added, that tests the skip heights _indirectly_, by doing a bunch of ancestor searches, and counting the number of steps needed, and doing some asserts on the step count. In a 200k long chain a series 1000 runs are performed. In each run a start and a target heights are picked randomly: the start is in the upper half of the chain, and the target is below it, by at lea
...