🤔 BrandonOdiwuor reviewed a pull request: "init: Correct coins db cache size setting"
(https://github.com/bitcoin/bitcoin/pull/31064#pullrequestreview-2396283480)
Code Review ACK 3a4a788ee0db83d20607f14801dbed2ee932943c
(https://github.com/bitcoin/bitcoin/pull/31064#pullrequestreview-2396283480)
Code Review ACK 3a4a788ee0db83d20607f14801dbed2ee932943c
📝 hebasto opened a pull request: "build, ci: Fix linking `bitcoin-chainstate.exe` to `bticoinkernel.dll` on Windows"
(https://github.com/bitcoin/bitcoin/pull/31158)
On the master branch @ 9a7206a34e3179d7280de98a818a13374178ee7b, linking `bitcoin-chainstate.exe` to `bticoinkernel.dll` fails:
```
> cmake -B build --preset vs2022-static -DBUILD_UTIL_CHAINSTATE=ON -DBUILD_KERNEL_LIB=ON -DBUILD_SHARED_LIBS=ON
> cmake --build build --config Release -j 8 -t bitcoin-chainstate
...
LINK : fatal error LNK1181: cannot open input file 'kernel\Release\bitcoinkernel.lib' [C:\Users\hebasto\source\repos\bitcoin\build-static\src\bitcoin-chainstate.vcxproj]
```
Thi
...
(https://github.com/bitcoin/bitcoin/pull/31158)
On the master branch @ 9a7206a34e3179d7280de98a818a13374178ee7b, linking `bitcoin-chainstate.exe` to `bticoinkernel.dll` fails:
```
> cmake -B build --preset vs2022-static -DBUILD_UTIL_CHAINSTATE=ON -DBUILD_KERNEL_LIB=ON -DBUILD_SHARED_LIBS=ON
> cmake --build build --config Release -j 8 -t bitcoin-chainstate
...
LINK : fatal error LNK1181: cannot open input file 'kernel\Release\bitcoinkernel.lib' [C:\Users\hebasto\source\repos\bitcoin\build-static\src\bitcoin-chainstate.vcxproj]
```
Thi
...
💬 TheCharlatan commented on pull request "build, ci: Fix linking `bitcoin-chainstate.exe` to `bitcoinkernel.dll` on Windows":
(https://github.com/bitcoin/bitcoin/pull/31158#discussion_r1817300867)
Do you want to take this opportunity to also add a better description to the test name? Maybe something like:
`Win64 native, VS 2022, no depends, libbitcoinkernel`?
(https://github.com/bitcoin/bitcoin/pull/31158#discussion_r1817300867)
Do you want to take this opportunity to also add a better description to the test name? Maybe something like:
`Win64 native, VS 2022, no depends, libbitcoinkernel`?
💬 achow101 commented on pull request "test: Assert that when we add the max orphan amount that we cannot add anymore and that a random orphan gets dropped":
(https://github.com/bitcoin/bitcoin/pull/31040#issuecomment-2438730450)
ACK 5c299ecafe6f336cffa145d28036b04b87e26712
(https://github.com/bitcoin/bitcoin/pull/31040#issuecomment-2438730450)
ACK 5c299ecafe6f336cffa145d28036b04b87e26712
💬 hebasto commented on pull request "build, ci: Fix linking `bitcoin-chainstate.exe` to `bitcoinkernel.dll` on Windows":
(https://github.com/bitcoin/bitcoin/pull/31158#discussion_r1817309322)
Thanks! "libbitcoinkernel" taken; it seems awkward to mention "depends", which is a non-Windows thing, on Windows :)
(https://github.com/bitcoin/bitcoin/pull/31158#discussion_r1817309322)
Thanks! "libbitcoinkernel" taken; it seems awkward to mention "depends", which is a non-Windows thing, on Windows :)
💬 TheCharlatan commented on pull request "build, ci: Fix linking `bitcoin-chainstate.exe` to `bitcoinkernel.dll` on Windows":
(https://github.com/bitcoin/bitcoin/pull/31158#discussion_r1817314019)
Ah, I was thinking of mentioning it because of the macOS build, but I guess we can have native macOS and depends, so it makes sense there, but as you say, does not make sense for msvc.
(https://github.com/bitcoin/bitcoin/pull/31158#discussion_r1817314019)
Ah, I was thinking of mentioning it because of the macOS build, but I guess we can have native macOS and depends, so it makes sense there, but as you say, does not make sense for msvc.
🚀 achow101 merged a pull request: "test: Assert that when we add the max orphan amount that we cannot add anymore and that a random orphan gets dropped"
(https://github.com/bitcoin/bitcoin/pull/31040)
(https://github.com/bitcoin/bitcoin/pull/31040)
💬 tdb3 commented on pull request "rpc: getorphantxs follow-up":
(https://github.com/bitcoin/bitcoin/pull/31043#issuecomment-2438925036)
Pushed to 38c2ef250817351919eebfbea29549c4f68efac1
- Renamed expiration variable to ORPHAN_TX_EXPIRE_TIME
- getorphantxs now rejects undefined verbosity (<0 and >2), and disallows boolean verbosity through an updated `ParseVerbosity()`.
(https://github.com/bitcoin/bitcoin/pull/31043#issuecomment-2438925036)
Pushed to 38c2ef250817351919eebfbea29549c4f68efac1
- Renamed expiration variable to ORPHAN_TX_EXPIRE_TIME
- getorphantxs now rejects undefined verbosity (<0 and >2), and disallows boolean verbosity through an updated `ParseVerbosity()`.
💬 tdb3 commented on pull request "rpc: getorphantxs follow-up":
(https://github.com/bitcoin/bitcoin/pull/31043#discussion_r1817400739)
fixed
(https://github.com/bitcoin/bitcoin/pull/31043#discussion_r1817400739)
fixed
👋 tdb3's pull request is ready for review: "test: enhance p2p_orphan_handling"
(https://github.com/bitcoin/bitcoin/pull/31037)
(https://github.com/bitcoin/bitcoin/pull/31037)
💬 kevkevinpal commented on pull request "test: added test to assert TX decode rpc error on submitpackage rpc":
(https://github.com/bitcoin/bitcoin/pull/31139#discussion_r1817630156)
I removed them in [1fab1d6](https://github.com/bitcoin/bitcoin/pull/31139/commits/1fab1d601e40edc7296cf3d60a7e917b0e6493b3)
(https://github.com/bitcoin/bitcoin/pull/31139#discussion_r1817630156)
I removed them in [1fab1d6](https://github.com/bitcoin/bitcoin/pull/31139/commits/1fab1d601e40edc7296cf3d60a7e917b0e6493b3)
💬 kevkevinpal commented on pull request "test: added test to assert TX decode rpc error on submitpackage rpc":
(https://github.com/bitcoin/bitcoin/pull/31139#discussion_r1817631565)
ok I added an assert to check for `txid` in `getrawmempool` in [1fab1d6](https://github.com/bitcoin/bitcoin/pull/31139/commits/1fab1d601e40edc7296cf3d60a7e917b0e6493b3)
let me know if that looks good to you!
(https://github.com/bitcoin/bitcoin/pull/31139#discussion_r1817631565)
ok I added an assert to check for `txid` in `getrawmempool` in [1fab1d6](https://github.com/bitcoin/bitcoin/pull/31139/commits/1fab1d601e40edc7296cf3d60a7e917b0e6493b3)
let me know if that looks good to you!
👍 hodlinator approved a pull request: "tinyformat: enforce compile-time checks for format string literals"
(https://github.com/bitcoin/bitcoin/pull/31149#pullrequestreview-2397082398)
re-ACK 23560b468c474bbc6a3abd6c0778da62766ac8f7
Changes since [previous ACK](https://github.com/bitcoin/bitcoin/pull/31149#pullrequestreview-2393618400):
- Added file-local `fuzz_fmt` function to reduce churn.
- Rebased and applied conversions to new code from #29936.
(https://github.com/bitcoin/bitcoin/pull/31149#pullrequestreview-2397082398)
re-ACK 23560b468c474bbc6a3abd6c0778da62766ac8f7
Changes since [previous ACK](https://github.com/bitcoin/bitcoin/pull/31149#pullrequestreview-2393618400):
- Added file-local `fuzz_fmt` function to reduce churn.
- Rebased and applied conversions to new code from #29936.
👍 rkrux approved a pull request: "test: added test to assert TX decode rpc error on submitpackage rpc"
(https://github.com/bitcoin/bitcoin/pull/31139#pullrequestreview-2397147428)
ACK 1fab1d601e40edc7296cf3d60a7e917b0e6493b3
Thanks for incorporating the changes.
(https://github.com/bitcoin/bitcoin/pull/31139#pullrequestreview-2397147428)
ACK 1fab1d601e40edc7296cf3d60a7e917b0e6493b3
Thanks for incorporating the changes.
🤔 sdaftuar reviewed a pull request: "Ephemeral Dust"
(https://github.com/bitcoin/bitcoin/pull/30239#pullrequestreview-2395962778)
Looks pretty good, I think most of my comments are nitty. Along those lines, you may want to retouch the commit message of fcbfed4630bdee6c52644c89e972ba7c130f47d5, as I think there's a missing paren.
(https://github.com/bitcoin/bitcoin/pull/30239#pullrequestreview-2395962778)
Looks pretty good, I think most of my comments are nitty. Along those lines, you may want to retouch the commit message of fcbfed4630bdee6c52644c89e972ba7c130f47d5, as I think there's a missing paren.
💬 sdaftuar commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1817094524)
Is this logging correct? The `parent_txid` here is really the txid of the child that didn't spend all of some parent's dust. I don't think we know which actual parent of that transaction had the unspent dust, unless we add more information to the return value of `CheckEphemeralSpends`.
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1817094524)
Is this logging correct? The `parent_txid` here is really the txid of the child that didn't spend all of some parent's dust. I don't think we know which actual parent of that transaction had the unspent dust, unless we add more information to the return value of `CheckEphemeralSpends`.
💬 sdaftuar commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1817374367)
Just want to make sure I understand what you mean by "sibling" here. Is the idea that if we have a topology like this:
```mermaid
flowchart TD
TxA --> TxC
TxB --> TxC
```
Where Tx A has ephemeral dust outputs and is in the mempool, and Tx C spends them and is in the mempool, than you want to return an outpoint of Tx B, whether or not Tx B itself is in the mempool, and whether or not Tx B itself has ephemeral dust outputs?
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1817374367)
Just want to make sure I understand what you mean by "sibling" here. Is the idea that if we have a topology like this:
```mermaid
flowchart TD
TxA --> TxC
TxB --> TxC
```
Where Tx A has ephemeral dust outputs and is in the mempool, and Tx C spends them and is in the mempool, than you want to return an outpoint of Tx B, whether or not Tx B itself is in the mempool, and whether or not Tx B itself has ephemeral dust outputs?
💬 sdaftuar commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1817315017)
I think all the parents in-mempool at this point?
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1817315017)
I think all the parents in-mempool at this point?
💬 sdaftuar commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1817772690)
Comment needs to be updated I think?
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1817772690)
Comment needs to be updated I think?
💬 sdaftuar commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1817223902)
Is it also worth testing that this fails if submitted via `sendrawtransaction` as well, so we're not just testing the `AcceptPackage()` code path?
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1817223902)
Is it also worth testing that this fails if submitted via `sendrawtransaction` as well, so we're not just testing the `AcceptPackage()` code path?