💬 maflcko commented on pull request "rpc: use CScheduler for relocking wallet and remove RPCTimer":
(https://github.com/bitcoin/bitcoin/pull/32862#issuecomment-3127574724)
It looks like a false positive, so I guess if you really want to fix it, you'll have to do it upstream.
(https://github.com/bitcoin/bitcoin/pull/32862#issuecomment-3127574724)
It looks like a false positive, so I guess if you really want to fix it, you'll have to do it upstream.
💬 darosior commented on pull request "Introduce per-txin sighash midstate cache for legacy/p2sh/segwitv0 scripts":
(https://github.com/bitcoin/bitcoin/pull/32473#issuecomment-3127602406)
I ran two identically compiled nodes on two identical VMs on the same host, one on top of fb7e9decf3f12ebae786e0cecf2f24a91c6e9d4c (this PR modified to enable caching for block validation too) and one on top of 2cad7226c2d02ec67bbac7ec909030f8bae593f8 (the commit it is based on).
Over 4 days of blocks (from height 906875 (`00000000000000000000264374e6d9b26be206793712e94266a62ddb0e9551e9`) to height 907450 (`00000000000000000001d85ba0ff861fd2499bd72fd436e2c37e11e0c28caf74`)), the average input
...
(https://github.com/bitcoin/bitcoin/pull/32473#issuecomment-3127602406)
I ran two identically compiled nodes on two identical VMs on the same host, one on top of fb7e9decf3f12ebae786e0cecf2f24a91c6e9d4c (this PR modified to enable caching for block validation too) and one on top of 2cad7226c2d02ec67bbac7ec909030f8bae593f8 (the commit it is based on).
Over 4 days of blocks (from height 906875 (`00000000000000000000264374e6d9b26be206793712e94266a62ddb0e9551e9`) to height 907450 (`00000000000000000001d85ba0ff861fd2499bd72fd436e2c37e11e0c28caf74`)), the average input
...
💬 ryanofsky commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-3127615193)
Thanks for the reviews!
Updated 3d099384a60c8f64c8d67a9f1e7b8649a9c54313 -> e1f139bd5fc9ee737d2b08307fca2b33354c5747 ([`pr/mine.29`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.29) -> [`pr/mine.30`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.30), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/mine.29..pr/mine.30)) just fixing comments to clarify things.
---
re: https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-3116367414
> It would be better to
...
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-3127615193)
Thanks for the reviews!
Updated 3d099384a60c8f64c8d67a9f1e7b8649a9c54313 -> e1f139bd5fc9ee737d2b08307fca2b33354c5747 ([`pr/mine.29`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.29) -> [`pr/mine.30`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.30), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/mine.29..pr/mine.30)) just fixing comments to clarify things.
---
re: https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-3116367414
> It would be better to
...
💬 ajtowns commented on pull request "refactor: GenTxid type safety followups":
(https://github.com/bitcoin/bitcoin/pull/33005#discussion_r2236858410)
Sounds fine.
Another option would be to move `info_for_relay` from txmempool.h into net_processing.cpp:
```c++
/** Returns info for a transaction if its entry_sequence < last_sequence */
static TxMempoolInfo info_for_relay(const CTxMemPool& mempool, const GenTxid& gtxid, uint64_t last_sequence)
{
return std::visit([&](const auto& id) {
LOCK(mempool.cs);
auto i{mempool.GetIter(id)};
return (i.has_value() && i.value()->GetSequence() < last_sequence) ? mempo
...
(https://github.com/bitcoin/bitcoin/pull/33005#discussion_r2236858410)
Sounds fine.
Another option would be to move `info_for_relay` from txmempool.h into net_processing.cpp:
```c++
/** Returns info for a transaction if its entry_sequence < last_sequence */
static TxMempoolInfo info_for_relay(const CTxMemPool& mempool, const GenTxid& gtxid, uint64_t last_sequence)
{
return std::visit([&](const auto& id) {
LOCK(mempool.cs);
auto i{mempool.GetIter(id)};
return (i.has_value() && i.value()->GetSequence() < last_sequence) ? mempo
...
👍 ryanofsky approved a pull request: "rpc: Handle -named argument parsing where '=' character is used"
(https://github.com/bitcoin/bitcoin/pull/32821#pullrequestreview-3063259562)
Code review ACK fa37153288ca420420636046ef6b8c4ba7e5a478. Thanks for the updates! Only change since last review was clarifying a comment and some variable names and adding new tests to cover cases where non-string positional parameters containing '=' are passed.
(https://github.com/bitcoin/bitcoin/pull/32821#pullrequestreview-3063259562)
Code review ACK fa37153288ca420420636046ef6b8c4ba7e5a478. Thanks for the updates! Only change since last review was clarifying a comment and some variable names and adding new tests to cover cases where non-string positional parameters containing '=' are passed.
👍 ryanofsky approved a pull request: "util: Abort on failing CHECK_NONFATAL in debug builds"
(https://github.com/bitcoin/bitcoin/pull/32588#pullrequestreview-3063286442)
Code review ACK fa37153288ca420420636046ef6b8c4ba7e5a478, just catching subprocess.CalledProcessError in test fixing up a comment since last review
(https://github.com/bitcoin/bitcoin/pull/32588#pullrequestreview-3063286442)
Code review ACK fa37153288ca420420636046ef6b8c4ba7e5a478, just catching subprocess.CalledProcessError in test fixing up a comment since last review
👍 pablomartin4btc approved a pull request: "doc: Add legacy wallet removal release notes"
(https://github.com/bitcoin/bitcoin/pull/33075#pullrequestreview-3063302337)
ACK fa45ccc15dfc52e798da62548dc43d1bd7889c9a
(https://github.com/bitcoin/bitcoin/pull/33075#pullrequestreview-3063302337)
ACK fa45ccc15dfc52e798da62548dc43d1bd7889c9a
🤔 janb84 reviewed a pull request: "doc: Add legacy wallet removal release notes"
(https://github.com/bitcoin/bitcoin/pull/33075#pullrequestreview-3063334927)
re ACK fa45ccc15dfc52e798da62548dc43d1bd7889c9a
changes since last ACK:
- removed double mention of `newkeypool `
(https://github.com/bitcoin/bitcoin/pull/33075#pullrequestreview-3063334927)
re ACK fa45ccc15dfc52e798da62548dc43d1bd7889c9a
changes since last ACK:
- removed double mention of `newkeypool `
💬 l0rinc commented on pull request "log: rate limiting followups":
(https://github.com/bitcoin/bitcoin/pull/33011#issuecomment-3127754875)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/33011#issuecomment-3127754875)
Concept ACK
⚠️ enirox001 opened an issue: "Build warnings from deprecated std::get_temporary_buffer in std::stable_sort calls"
(https://github.com/bitcoin/bitcoin/issues/33080)
### Is there an existing issue for this?
- [x] I have searched the existing issues
### Current behaviour
Clang 20 generates deprecation warnings when std::stable_sort internally calls std::get_temporary_buffer (deprecated in C++17, removed in C++26).
### Expected behaviour
Clean builds without deprecation warnings.
### Steps to reproduce
Compile with Clang 20 (observed during fuzz build):
```
cmake --preset=libfuzzer
cmake --build build_fuzz
```
### Relevant log output
```
warning: 'ge
...
(https://github.com/bitcoin/bitcoin/issues/33080)
### Is there an existing issue for this?
- [x] I have searched the existing issues
### Current behaviour
Clang 20 generates deprecation warnings when std::stable_sort internally calls std::get_temporary_buffer (deprecated in C++17, removed in C++26).
### Expected behaviour
Clean builds without deprecation warnings.
### Steps to reproduce
Compile with Clang 20 (observed during fuzz build):
```
cmake --preset=libfuzzer
cmake --build build_fuzz
```
### Relevant log output
```
warning: 'ge
...
💬 willcl-ark commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2237012262)
We did try this originally, but (very annoyingly) it does not seem to be possible without a primary workflow to call this workflow. The reason is that the `runs-on` field only has access to the following "contexts":
```
github, needs, strategy, matrix, vars, inputs
```
See https://docs.github.com/en/actions/reference/workflows-and-actions/contexts
Whilst `vars` sounds like it would do the job, the problem is that in workflows of the type `on: pull_request` that `vars` (which refer her
...
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2237012262)
We did try this originally, but (very annoyingly) it does not seem to be possible without a primary workflow to call this workflow. The reason is that the `runs-on` field only has access to the following "contexts":
```
github, needs, strategy, matrix, vars, inputs
```
See https://docs.github.com/en/actions/reference/workflows-and-actions/contexts
Whilst `vars` sounds like it would do the job, the problem is that in workflows of the type `on: pull_request` that `vars` (which refer her
...
💬 murchandamus commented on pull request "Reduce minrelaytxfee to 100 sats/kvB":
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3127923208)
@RobinLinus, are you planning on working on this?
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3127923208)
@RobinLinus, are you planning on working on this?
💬 theStack commented on pull request "p2p: TxOrphanage revamp cleanups":
(https://github.com/bitcoin/bitcoin/pull/32941#issuecomment-3127965996)
Concept ACK
Makes sense to keep the maxorphantx option for a while to only show a warning. Changes look good at first glance, will do a deeper dive with focus on test and fuzzing changes (considering https://github.com/bitcoin/bitcoin/pull/32941#issuecomment-3120296633) during the week.
(https://github.com/bitcoin/bitcoin/pull/32941#issuecomment-3127965996)
Concept ACK
Makes sense to keep the maxorphantx option for a while to only show a warning. Changes look good at first glance, will do a deeper dive with focus on test and fuzzing changes (considering https://github.com/bitcoin/bitcoin/pull/32941#issuecomment-3120296633) during the week.
📝 fanquake opened a pull request: "[NO MERGE]: TSAN should fail"
(https://github.com/bitcoin/bitcoin/pull/33081)
Ref: https://github.com/bitcoin/bitcoin/pull/32862#issuecomment-3114706601.
(https://github.com/bitcoin/bitcoin/pull/33081)
Ref: https://github.com/bitcoin/bitcoin/pull/32862#issuecomment-3114706601.
💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3128011661)
Updated 1ffc1c9d94b16cdbfb92a26d0f0e75451efad4fe -> 938767d957b7669accfb554a7cbb25141f7e8632 ([kernelApi_45](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_45) -> [kernelApi_46](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_46), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_45..kernelApi_46))
* Fixed symbol visibility for windows static builds and simplified the macro checks in the header a bit.
* Removed the kernel library symbol visibility hack, this
...
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3128011661)
Updated 1ffc1c9d94b16cdbfb92a26d0f0e75451efad4fe -> 938767d957b7669accfb554a7cbb25141f7e8632 ([kernelApi_45](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_45) -> [kernelApi_46](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_46), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_45..kernelApi_46))
* Fixed symbol visibility for windows static builds and simplified the macro checks in the header a bit.
* Removed the kernel library symbol visibility hack, this
...
🤔 stickies-v reviewed a pull request: "kernel: Introduce initial C header API"
(https://github.com/bitcoin/bitcoin/pull/30595#pullrequestreview-3030139760)
Did another review round while updating py-bitcoinkernel. The main themes are:
- using reference counting internally to let us simplify the interface (e.g. expose `kernel_TransactionUndo` and `kernel_Coin` instead of letting the user manage indexes) as well as replace expensive `_copy` operations with `_get` ones
- note: in the WG we have discussed exposing reference counting through the public interface too, but personally I don't see the benefits for that complexity yet, even if I'm happy
...
(https://github.com/bitcoin/bitcoin/pull/30595#pullrequestreview-3030139760)
Did another review round while updating py-bitcoinkernel. The main themes are:
- using reference counting internally to let us simplify the interface (e.g. expose `kernel_TransactionUndo` and `kernel_Coin` instead of letting the user manage indexes) as well as replace expensive `_copy` operations with `_get` ones
- note: in the WG we have discussed exposing reference counting through the public interface too, but personally I don't see the benefits for that complexity yet, even if I'm happy
...
💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2214073393)
clang-format sorts this alphabetically, but `windows.h` needs to be included before `shellapi.h` otherwise we get [build errors](https://github.com/stickies-v/bitcoin/actions/runs/16352515776/job/46202676758#step:10:953) like:
```
C:\Program Files (x86)\Windows Kits\10\Include\10.0.20348.0\um\shellapi.h(68,1): error C2146: syntax error: missing ';' before identifier 'DECLSPEC_IMPORT' [D:\a\bitcoin\bitcoin\build\src\bitcoin-chainstate.vcxproj]
(compiling source file '../../src/bitcoin-chai
...
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2214073393)
clang-format sorts this alphabetically, but `windows.h` needs to be included before `shellapi.h` otherwise we get [build errors](https://github.com/stickies-v/bitcoin/actions/runs/16352515776/job/46202676758#step:10:953) like:
```
C:\Program Files (x86)\Windows Kits\10\Include\10.0.20348.0\um\shellapi.h(68,1): error C2146: syntax error: missing ';' before identifier 'DECLSPEC_IMPORT' [D:\a\bitcoin\bitcoin\build\src\bitcoin-chainstate.vcxproj]
(compiling source file '../../src/bitcoin-chai
...
💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2213968769)
Should this be const? It's not moveable as-is. If intentional, brief docstring would be good?
```suggestion
std::unique_ptr<kernel_BlockUndo, Deleter> m_block_undo;
```
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2213968769)
Should this be const? It's not moveable as-is. If intentional, brief docstring would be good?
```suggestion
std::unique_ptr<kernel_BlockUndo, Deleter> m_block_undo;
```
💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2213740015)
nit: couple of naming mismatches after latest force pushes:
<details>
<summary>git diff on 1ffc1c9d94</summary>
```diff
diff --git a/src/kernel/bitcoinkernel.h b/src/kernel/bitcoinkernel.h
index b72c001d1b..ec4db4e7c7 100644
--- a/src/kernel/bitcoinkernel.h
+++ b/src/kernel/bitcoinkernel.h
@@ -130,7 +130,7 @@ typedef struct kernel_TransactionOutput kernel_TransactionOutput;
*
* Messages that were logged before a connection is created are buffered in a
* 1MB buffer. Logging c
...
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2213740015)
nit: couple of naming mismatches after latest force pushes:
<details>
<summary>git diff on 1ffc1c9d94</summary>
```diff
diff --git a/src/kernel/bitcoinkernel.h b/src/kernel/bitcoinkernel.h
index b72c001d1b..ec4db4e7c7 100644
--- a/src/kernel/bitcoinkernel.h
+++ b/src/kernel/bitcoinkernel.h
@@ -130,7 +130,7 @@ typedef struct kernel_TransactionOutput kernel_TransactionOutput;
*
* Messages that were logged before a connection is created are buffered in a
* 1MB buffer. Logging c
...
💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2216084200)
This looks like it leaks memory since we never deallocate `block`. Perhaps using `std::unique_ptr` here is a better approach? This (+`kernel_block_undo_read`) seems like the most dangerous one, but perhaps good practice to do this in other places we allocate memory, e.g. [here](https://github.com/bitcoin/bitcoin/pull/30595/files#diff-cc28221ef8d0c7294dda4e3df9f70bb6c062006b387468380c2c2cc02b6762c3R421).
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2216084200)
This looks like it leaks memory since we never deallocate `block`. Perhaps using `std::unique_ptr` here is a better approach? This (+`kernel_block_undo_read`) seems like the most dangerous one, but perhaps good practice to do this in other places we allocate memory, e.g. [here](https://github.com/bitcoin/bitcoin/pull/30595/files#diff-cc28221ef8d0c7294dda4e3df9f70bb6c062006b387468380c2c2cc02b6762c3R421).