💬 mzumsande commented on pull request "validation: Add eligible ancestors of reconsidered block to setBlockIndexCandidates":
(https://github.com/bitcoin/bitcoin/pull/30479#issuecomment-2892651335)
Looks like there is a intermittent failure in the added test - I'll look into it soon.
(https://github.com/bitcoin/bitcoin/pull/30479#issuecomment-2892651335)
Looks like there is a intermittent failure in the added test - I'll look into it soon.
💬 luke-jr commented on pull request "Mining: Avoid copying template CBlocks":
(https://github.com/bitcoin/bitcoin/pull/32547#issuecomment-2892721133)
>In general it doesn't make sense to mark virtual methods const, because parent classes can't know how subclasses will implement the virtual methods and what state they may access. The point of pure virtual methods is usually to declare abstract interfaces without leaking information about implementations or making assumptions about them.
The constness of methods is a property of the interface. It is a way to say calling it will not change the object, and thereby allows using const references
...
(https://github.com/bitcoin/bitcoin/pull/32547#issuecomment-2892721133)
>In general it doesn't make sense to mark virtual methods const, because parent classes can't know how subclasses will implement the virtual methods and what state they may access. The point of pure virtual methods is usually to declare abstract interfaces without leaking information about implementations or making assumptions about them.
The constness of methods is a property of the interface. It is a way to say calling it will not change the object, and thereby allows using const references
...
✅ maflcko closed a pull request: "doc: Fix gen-manpages to check build options"
(https://github.com/bitcoin/bitcoin/pull/29457)
(https://github.com/bitcoin/bitcoin/pull/29457)
💬 maflcko commented on pull request "Remove legacy `Parse(U)Int*`":
(https://github.com/bitcoin/bitcoin/pull/32520#issuecomment-2892971961)
thx, I pushed a commit similar to yours. (Generally, when it comes to the dev notes, I think it is clear that no one is reading them, because of all the stale references in it. We should probably work on reducing them to only contain the important stuff that is not already checked by the automated linters or not already covered by other docs)
(https://github.com/bitcoin/bitcoin/pull/32520#issuecomment-2892971961)
thx, I pushed a commit similar to yours. (Generally, when it comes to the dev notes, I think it is clear that no one is reading them, because of all the stale references in it. We should probably work on reducing them to only contain the important stuff that is not already checked by the automated linters or not already covered by other docs)
💬 polespinasa commented on pull request "rpc: Round verificationprogress to 1 for a recent tip":
(https://github.com/bitcoin/bitcoin/pull/32528#discussion_r2097026058)
> But doesn't the current code already solve the problem?
No, maflcko proposal returns `verificationprogress=1` when the last verified block is within a 2h offset of our local time.
(https://github.com/bitcoin/bitcoin/pull/32528#discussion_r2097026058)
> But doesn't the current code already solve the problem?
No, maflcko proposal returns `verificationprogress=1` when the last verified block is within a 2h offset of our local time.
💬 maflcko commented on pull request "rpc: Round verificationprogress to 1 for a recent tip":
(https://github.com/bitcoin/bitcoin/pull/32528#discussion_r2097134143)
> lie to users
Again, this can happen on current master already. Also, it can happen with your suggested diff. The only reliable way to not rely on the exact value of remotely miner-set timestamps would be to not use them, but that is a more involved patch.
(https://github.com/bitcoin/bitcoin/pull/32528#discussion_r2097134143)
> lie to users
Again, this can happen on current master already. Also, it can happen with your suggested diff. The only reliable way to not rely on the exact value of remotely miner-set timestamps would be to not use them, but that is a more involved patch.
💬 polespinasa commented on pull request "rpc: Round verificationprogress to 1 for a recent tip":
(https://github.com/bitcoin/bitcoin/pull/32528#discussion_r2097152746)
> Also, it can happen with your suggested diff
Oh, you're right if the miner sets the timestamp to a future one. Maybe we could just limit the maximum value to something like `0.999...` until we verified the last block. That's hard coding a bit the values, but as it's only for a greater UX I don't see much problem doing that.
(https://github.com/bitcoin/bitcoin/pull/32528#discussion_r2097152746)
> Also, it can happen with your suggested diff
Oh, you're right if the miner sets the timestamp to a future one. Maybe we could just limit the maximum value to something like `0.999...` until we verified the last block. That's hard coding a bit the values, but as it's only for a greater UX I don't see much problem doing that.
💬 Sjors commented on pull request "Add checkBlock() to Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31981#discussion_r2097179087)
It seems Python doesn't believe in `const`, so I ended up just documenting the test and its call site.
(https://github.com/bitcoin/bitcoin/pull/31981#discussion_r2097179087)
It seems Python doesn't believe in `const`, so I ended up just documenting the test and its call site.
💬 Sjors commented on pull request "Add checkBlock() to Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31981#discussion_r2097200882)
I dropped the changes to `rpc/mining.cpp` and explained why they're untouched in the commit message. Indeed `checkBlock` is already covered by the unit tests, and the meat and potatoes is in `TestBlockValidity`, which both IPC and RPC call.
(https://github.com/bitcoin/bitcoin/pull/31981#discussion_r2097200882)
I dropped the changes to `rpc/mining.cpp` and explained why they're untouched in the commit message. Indeed `checkBlock` is already covered by the unit tests, and the meat and potatoes is in `TestBlockValidity`, which both IPC and RPC call.
💬 Sjors commented on pull request "Add checkBlock() to Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31981#discussion_r2097201740)
(note: I'll push this in a few hours, just in case you're up early...)
(https://github.com/bitcoin/bitcoin/pull/31981#discussion_r2097201740)
(note: I'll push this in a few hours, just in case you're up early...)
💬 Sjors commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2097218973)
Well that's what confused me. My first thought was: why does this work if we're not checking the witness?
But maybe the sigops related comment needs to go above the "only the non-witness" portion.
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2097218973)
Well that's what confused me. My first thought was: why does this work if we're not checking the witness?
But maybe the sigops related comment needs to go above the "only the non-witness" portion.
💬 Sjors commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2097226712)
I don't think the 3 character savings is worth making people jump to the documentation. `MAX_TX_` vs `MAX_BLOCK_` seems like a good convention to keep.
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2097226712)
I don't think the 3 character savings is worth making people jump to the documentation. `MAX_TX_` vs `MAX_BLOCK_` seems like a good convention to keep.
💬 Sjors commented on pull request "test: properly check for per-tx sigops limit":
(https://github.com/bitcoin/bitcoin/pull/32533#issuecomment-2893307461)
I was thinking ahead to when BIP54 is activated, but yes, "tightening".
(https://github.com/bitcoin/bitcoin/pull/32533#issuecomment-2893307461)
I was thinking ahead to when BIP54 is activated, but yes, "tightening".
💬 davidgumberg commented on pull request "deps: Bump lief to 0.16.4":
(https://github.com/bitcoin/bitcoin/pull/32431#issuecomment-2893315569)
> I have a local manifest which builds 0.16.3 lief: https://gist.github.com/willcl-ark/3ac07722025b696910adba256d813cb6
I was using basically this manifest taken from your branch (https://github.com/willcl-ark/bitcoin/tree/bump-lief), but it seems some build changes were made in lief `0.16.5` that prevent this from working, I've rebased, bumping to `0.16.4`
(https://github.com/bitcoin/bitcoin/pull/32431#issuecomment-2893315569)
> I have a local manifest which builds 0.16.3 lief: https://gist.github.com/willcl-ark/3ac07722025b696910adba256d813cb6
I was using basically this manifest taken from your branch (https://github.com/willcl-ark/bitcoin/tree/bump-lief), but it seems some build changes were made in lief `0.16.5` that prevent this from working, I've rebased, bumping to `0.16.4`
👋 davidgumberg's pull request is ready for review: "deps: Bump lief to 0.16.4"
(https://github.com/bitcoin/bitcoin/pull/32431)
(https://github.com/bitcoin/bitcoin/pull/32431)
💬 laanwj commented on pull request "Set minimum supported Windows version to 1903 (May 2019 Update)":
(https://github.com/bitcoin/bitcoin/pull/32537#discussion_r2097253630)
Imo, no need to do this as a separate step. Might as well roll the update to the release notes template into #32380.
(https://github.com/bitcoin/bitcoin/pull/32537#discussion_r2097253630)
Imo, no need to do this as a separate step. Might as well roll the update to the release notes template into #32380.
💬 laanwj commented on pull request "Modernize use of UTF-8 in Windows code":
(https://github.com/bitcoin/bitcoin/pull/32380#discussion_r2097259314)
Can we maybe check the codepage early at startup (e.g. `SetupEnvironment()`) and fail if it isn't correct? This seems more thorough than any version check.
(https://github.com/bitcoin/bitcoin/pull/32380#discussion_r2097259314)
Can we maybe check the codepage early at startup (e.g. `SetupEnvironment()`) and fail if it isn't correct? This seems more thorough than any version check.
👍 fanquake approved a pull request: "lint: Check for missing trailing newline"
(https://github.com/bitcoin/bitcoin/pull/32477#pullrequestreview-2853191950)
ACK fa9198af55df74b0c19c9125d256ad4df83cf005
(https://github.com/bitcoin/bitcoin/pull/32477#pullrequestreview-2853191950)
ACK fa9198af55df74b0c19c9125d256ad4df83cf005
👍 hodlinator approved a pull request: "[IBD] multi-byte block obfuscation"
(https://github.com/bitcoin/bitcoin/pull/31144#pullrequestreview-2852896828)
ACK 989537ff4026d7c3fa5ba99701e0a4b134d950f7
Optimizing by operating on whole-word units instead of byte-for-byte makes for more efficient use of common hardware. Extracting the optimization into the `Obfuscation` abstraction which caches whole-word "rotations" (the XOR-key pre-rotated at all possible byte-offsets) makes sense.
Clear speedup in benchmarks. Even if the least improved of those measured (`ReadBlockBench`) would be the most representative of real-world use, it is sped up by ~1
...
(https://github.com/bitcoin/bitcoin/pull/31144#pullrequestreview-2852896828)
ACK 989537ff4026d7c3fa5ba99701e0a4b134d950f7
Optimizing by operating on whole-word units instead of byte-for-byte makes for more efficient use of common hardware. Extracting the optimization into the `Obfuscation` abstraction which caches whole-word "rotations" (the XOR-key pre-rotated at all possible byte-offsets) makes sense.
Clear speedup in benchmarks. Even if the least improved of those measured (`ReadBlockBench`) would be the most representative of real-world use, it is sped up by ~1
...
💬 hodlinator commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2097124059)
nit: Could update Godbolt-link in PR description to a newer version of `Obfuscation` with cached rotations?
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2097124059)
nit: Could update Godbolt-link in PR description to a newer version of `Obfuscation` with cached rotations?