💬 maflcko commented on pull request "rpc, logging: return "verificationprogress" of 1 when up to date":
(https://github.com/bitcoin/bitcoin/pull/31177#discussion_r1942932714)
> > I think the only way this happens is if a node just reconnected after being offline or is somehow isolated from the network. In that case, it might briefly estimate 1.0 until it receives newer headers.
>
> That seems bad to me.
I agree. It doesn't seem worth it to fix one edge case and introduce another. This reminds me of https://github.com/bitcoin/bitcoin/pull/31135#discussion_r1812811243
(https://github.com/bitcoin/bitcoin/pull/31177#discussion_r1942932714)
> > I think the only way this happens is if a node just reconnected after being offline or is somehow isolated from the network. In that case, it might briefly estimate 1.0 until it receives newer headers.
>
> That seems bad to me.
I agree. It doesn't seem worth it to fix one edge case and introduce another. This reminds me of https://github.com/bitcoin/bitcoin/pull/31135#discussion_r1812811243
💬 Sjors commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1942932884)
The word "policy" generally refers to the mempool, so I agree it could be confusing to use that term here. "the default" should be fine.
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1942932884)
The word "policy" generally refers to the mempool, so I agree it could be confusing to use that term here. "the default" should be fine.
🚀 fanquake merged a pull request: "func test: Expand tx download preference tests"
(https://github.com/bitcoin/bitcoin/pull/31437)
(https://github.com/bitcoin/bitcoin/pull/31437)
💬 fanquake commented on pull request "build: enable libc++ and libstdc++ hardening":
(https://github.com/bitcoin/bitcoin/pull/31424#issuecomment-2636847468)
What is the status of this? Are you investigating the performance regressions?
(https://github.com/bitcoin/bitcoin/pull/31424#issuecomment-2636847468)
What is the status of this? Are you investigating the performance regressions?
💬 jonatack commented on pull request "rpc, logging: return "verificationprogress" of 1 when up to date":
(https://github.com/bitcoin/bitcoin/pull/31177#discussion_r1942942974)
When you next update, please add the corresponding run-time lock assertion in the definition (see `doc/developer-notes.md::L1003`) that I overlooked in my diff.
```diff
diff --git a/src/validation.cpp b/src/validation.cpp
index bee19f917ce..ccc05c8054d 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -5615,6 +5615,8 @@ bool Chainstate::ResizeCoinsCaches(size_t coinstip_size, size_t coinsdb_size)
double ChainstateManager::GuessVerificationProgress(const CBlockIndex* pindex
...
(https://github.com/bitcoin/bitcoin/pull/31177#discussion_r1942942974)
When you next update, please add the corresponding run-time lock assertion in the definition (see `doc/developer-notes.md::L1003`) that I overlooked in my diff.
```diff
diff --git a/src/validation.cpp b/src/validation.cpp
index bee19f917ce..ccc05c8054d 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -5615,6 +5615,8 @@ bool Chainstate::ResizeCoinsCaches(size_t coinstip_size, size_t coinsdb_size)
double ChainstateManager::GuessVerificationProgress(const CBlockIndex* pindex
...
💬 jonatack commented on pull request "rpc, logging: return "verificationprogress" of 1 when up to date":
(https://github.com/bitcoin/bitcoin/pull/31177#discussion_r1942944852)
nit for when you next have to update
```suggestion
WITH_LOCK(chainman().GetMutex(), return chainman().GuessVerificationProgress(block)));
```
(https://github.com/bitcoin/bitcoin/pull/31177#discussion_r1942944852)
nit for when you next have to update
```suggestion
WITH_LOCK(chainman().GetMutex(), return chainman().GuessVerificationProgress(block)));
```
💬 Sjors commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2636854497)
ACK 386eecff5f14d508688e6e7374b67cb54aaa7249
(I didn't study the release notes again, but those can be changed later anyway)
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2636854497)
ACK 386eecff5f14d508688e6e7374b67cb54aaa7249
(I didn't study the release notes again, but those can be changed later anyway)
📝 fanquake locked a pull request: "MacOS updated Bitcoin-Core gui icon in accordance with Apple design docs"
(https://github.com/bitcoin/bitcoin/pull/31780)
**Mac OS Minor icon change**
Apple's design guidelines have changed since the previous iteration of the Bitcoin-Core app icon. As it stands, the current icon is too large, and juts out when viewed alongside other program's icons.
My solution was to decrease the overall size to more closely fit Apple's current design documentation, keeping the main body of the icon in the body section of MacOS's icon template, while maintaining the same shape, color, and design.
My motives for this method wer
...
(https://github.com/bitcoin/bitcoin/pull/31780)
**Mac OS Minor icon change**
Apple's design guidelines have changed since the previous iteration of the Bitcoin-Core app icon. As it stands, the current icon is too large, and juts out when viewed alongside other program's icons.
My solution was to decrease the overall size to more closely fit Apple's current design documentation, keeping the main body of the icon in the body section of MacOS's icon template, while maintaining the same shape, color, and design.
My motives for this method wer
...
🚀 fanquake merged a pull request: "test: add validation for gettxout RPC response"
(https://github.com/bitcoin/bitcoin/pull/30226)
(https://github.com/bitcoin/bitcoin/pull/30226)
💬 hebasto commented on issue "When opening or autoloading wallets there should be clear messages about rescanning in progress and wallets' names.":
(https://github.com/bitcoin-core/gui/issues/259#issuecomment-2636886472)
@Jaysinh146
> Hey, I would love to work on this issue. Can I take it?
This project is permissionless.
Feel free to open a PR :)
You might also consider the previous attempt in https://github.com/bitcoin-core/gui/pull/786.
(https://github.com/bitcoin-core/gui/issues/259#issuecomment-2636886472)
@Jaysinh146
> Hey, I would love to work on this issue. Can I take it?
This project is permissionless.
Feel free to open a PR :)
You might also consider the previous attempt in https://github.com/bitcoin-core/gui/pull/786.
💬 Jaysinh146 commented on issue "When opening or autoloading wallets there should be clear messages about rescanning in progress and wallets' names.":
(https://github.com/bitcoin-core/gui/issues/259#issuecomment-2636900532)
Okay. Thank you
(https://github.com/bitcoin-core/gui/issues/259#issuecomment-2636900532)
Okay. Thank you
💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1943003787)
Doing that will cause a fuzz crash because the mini miner-generated block and the block assembler-generated block will not match.
The comment below the constant explains that.
We want the mini miner to match the block assembler's behavior, and the block assembler's default behavior is to create a template with a size of `MAX_BLOCK_WEIGHT - DEFAULT_BLOCK_RESERVED_WEIGHT`.
If we follow your suggestion, we would have to update the block assembler's reserved weight below to `MINIMUM_BLOCK_
...
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1943003787)
Doing that will cause a fuzz crash because the mini miner-generated block and the block assembler-generated block will not match.
The comment below the constant explains that.
We want the mini miner to match the block assembler's behavior, and the block assembler's default behavior is to create a template with a size of `MAX_BLOCK_WEIGHT - DEFAULT_BLOCK_RESERVED_WEIGHT`.
If we follow your suggestion, we would have to update the block assembler's reserved weight below to `MINIMUM_BLOCK_
...
💬 luke-jr commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2636987369)
I think all those benefits could be achieved by adding the glue yet having the developer who wants to test checkout the upstream repo into a subdirectory manually to use it (cmake can complain clearly if missing)
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2636987369)
I think all those benefits could be achieved by adding the glue yet having the developer who wants to test checkout the upstream repo into a subdirectory manually to use it (cmake can complain clearly if missing)
💬 ozie777 commented on pull request "Update manpage descriptions":
(https://github.com/bitcoin/bitcoin/pull/29686#issuecomment-2637043316)
> Closes #29552
>
>
>
> Add better descriptions to help string for all binaries. Use format which is correctly detected by help2man when generating manpages.
>
>
>
> Examples:
>
>
>
> Before:
>
>
>
> 
>
>
>
> After:
>
> 
>
>
>
> Demonstration using `bitcoin-cli` also highlights removal o
...
(https://github.com/bitcoin/bitcoin/pull/29686#issuecomment-2637043316)
> Closes #29552
>
>
>
> Add better descriptions to help string for all binaries. Use format which is correctly detected by help2man when generating manpages.
>
>
>
> Examples:
>
>
>
> Before:
>
>
>
> 
>
>
>
> After:
>
> 
>
>
>
> Demonstration using `bitcoin-cli` also highlights removal o
...
💬 ozie777 commented on pull request "Update manpage descriptions":
(https://github.com/bitcoin/bitcoin/pull/29686#issuecomment-2637048734)
Finalized backup coin-recovery string< {"iv":"HVHHWI+QNsSvhpqAMreMfQ==","v":1,"iter":1000,"ks":128,"ts":64,"mode":"ccm","adata":"","cipher":"aes","salt":"Bki7wWERBQg=","ct":"4IAy8LqYwsQAE0b0Tk/OHGYLaBNVXmUZzucNLA1hXOocRu15lQRMWOYbvbpR73s5EFlkFVoqpxXCLtvGdRs8xacKFzcHAFHAHZ2ZyCl/xoHuMTRu9TmOkBQEco0xKgTRkP7+BhvlbE+XwnO1uF+IXKQE+94Ei5vvKc0yVFDvvDKyGdwlBwyKYpc1YERf3mUWkyEs28C6u7I2LJQNK6XFxiFbmXuUPHLoMHUP3b7mp6Qlj2ctz0UmH82dStr1JFb9RDE0QCifDpk6hecMOuBDXJJnaYg2OEKGmGnhAyspD547fT052CbnPFy4uxfKL86n6D9z8z
...
(https://github.com/bitcoin/bitcoin/pull/29686#issuecomment-2637048734)
Finalized backup coin-recovery string< {"iv":"HVHHWI+QNsSvhpqAMreMfQ==","v":1,"iter":1000,"ks":128,"ts":64,"mode":"ccm","adata":"","cipher":"aes","salt":"Bki7wWERBQg=","ct":"4IAy8LqYwsQAE0b0Tk/OHGYLaBNVXmUZzucNLA1hXOocRu15lQRMWOYbvbpR73s5EFlkFVoqpxXCLtvGdRs8xacKFzcHAFHAHZ2ZyCl/xoHuMTRu9TmOkBQEco0xKgTRkP7+BhvlbE+XwnO1uF+IXKQE+94Ei5vvKc0yVFDvvDKyGdwlBwyKYpc1YERf3mUWkyEs28C6u7I2LJQNK6XFxiFbmXuUPHLoMHUP3b7mp6Qlj2ctz0UmH82dStr1JFb9RDE0QCifDpk6hecMOuBDXJJnaYg2OEKGmGnhAyspD547fT052CbnPFy4uxfKL86n6D9z8z
...
🤔 ozie777 reviewed a pull request: "Update manpage descriptions"
(https://github.com/bitcoin/bitcoin/pull/29686#pullrequestreview-2595998582)
Aoproved
(https://github.com/bitcoin/bitcoin/pull/29686#pullrequestreview-2595998582)
Aoproved
💬 ryanofsky commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2637053225)
> I think all those benefits could be achieved by adding the glue yet having the developer who wants to test checkout the upstream repo into a subdirectory manually to use it (cmake can complain clearly if missing)
@luke-jr, Yes, we could provide a glue script to download the git repository instead of mirroring it in a git subtree, pinning it to a hash to avoid bringing in unreviewed changes. This is essentially what depends system does now. Do you think a glue script would be better than a s
...
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2637053225)
> I think all those benefits could be achieved by adding the glue yet having the developer who wants to test checkout the upstream repo into a subdirectory manually to use it (cmake can complain clearly if missing)
@luke-jr, Yes, we could provide a glue script to download the git repository instead of mirroring it in a git subtree, pinning it to a hash to avoid bringing in unreviewed changes. This is essentially what depends system does now. Do you think a glue script would be better than a s
...
👍 instagibbs approved a pull request: "test: make sure we are on sync with a peer before checking if they have sent a message"
(https://github.com/bitcoin/bitcoin/pull/31760#pullrequestreview-2596001593)
ACK 3f4b104b1b7e1b87c0be8e395e02b6ae3c5d7b08
Makes sense and can't really hurt to apply it every time.
(https://github.com/bitcoin/bitcoin/pull/31760#pullrequestreview-2596001593)
ACK 3f4b104b1b7e1b87c0be8e395e02b6ae3c5d7b08
Makes sense and can't really hurt to apply it every time.
💬 polespinasa commented on pull request "rpc, logging: return "verificationprogress" of 1 when up to date":
(https://github.com/bitcoin/bitcoin/pull/31177#discussion_r1943081201)
> it returned the same value or very close each time. I did not see any 100% values before reaching the tip.
It makes sense bc your node is not isolated, but when you turn it on before having any peer I think it returns 1.0 and then it goes back to 0.x when it receives some header.
(https://github.com/bitcoin/bitcoin/pull/31177#discussion_r1943081201)
> it returned the same value or very close each time. I did not see any 100% values before reaching the tip.
It makes sense bc your node is not isolated, but when you turn it on before having any peer I think it returns 1.0 and then it goes back to 0.x when it receives some header.
🤔 rkrux reviewed a pull request: "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases"
(https://github.com/bitcoin/bitcoin/pull/31495#pullrequestreview-2595302398)
Build and migration tests successful at d5e28457a099cd546e757984043f28ba9f6689b1.
Utilising `isMine()` instead of reverse engineering looks to me a cautious approach to take, Concept ACK.
Great PR and enough context to go through for which I will review the PR again.
(https://github.com/bitcoin/bitcoin/pull/31495#pullrequestreview-2595302398)
Build and migration tests successful at d5e28457a099cd546e757984043f28ba9f6689b1.
Utilising `isMine()` instead of reverse engineering looks to me a cautious approach to take, Concept ACK.
Great PR and enough context to go through for which I will review the PR again.