✅ maflcko closed a pull request: "rpc: Allow fullrbf fee bump in (psbt)bumpfee"
(https://github.com/bitcoin/bitcoin/pull/31953)
(https://github.com/bitcoin/bitcoin/pull/31953)
📝 maflcko reopened a pull request: "rpc: Allow fullrbf fee bump in (psbt)bumpfee"
(https://github.com/bitcoin/bitcoin/pull/31953)
The RPCs (psbt)bumpfee, and the GUI, reject fee bumps when BIP 125 signalling is absent in the transaction even when the mempool and other RPCs allow them. Fix the confusion by allowing the fee bump.
This is done after fullrbf is always on (https://github.com/bitcoin/bitcoin/pull/30592)
(https://github.com/bitcoin/bitcoin/pull/31953)
The RPCs (psbt)bumpfee, and the GUI, reject fee bumps when BIP 125 signalling is absent in the transaction even when the mempool and other RPCs allow them. Fix the confusion by allowing the fee bump.
This is done after fullrbf is always on (https://github.com/bitcoin/bitcoin/pull/30592)
💬 yancyribbens commented on pull request "bitcoin-cli: Add -ipcconnect option":
(https://github.com/bitcoin/bitcoin/pull/32297#issuecomment-2816691663)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/32297#issuecomment-2816691663)
Concept ACK
💬 yancyribbens commented on pull request "qt, wallet: Convert uint256 to Txid":
(https://github.com/bitcoin/bitcoin/pull/32238#issuecomment-2816692394)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/32238#issuecomment-2816692394)
Concept ACK
💬 luisschwab commented on pull request "wallet: make coinbase that will mature on the next block available for selection":
(https://github.com/bitcoin/bitcoin/pull/32123#discussion_r2051498528)
The `LOCK` is unlocked when it goes out of scope, so at L33 it's gets unlocked.
(https://github.com/bitcoin/bitcoin/pull/32123#discussion_r2051498528)
The `LOCK` is unlocked when it goes out of scope, so at L33 it's gets unlocked.
💬 andrewtoth commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2051507464)
I don't understand why these lines are commented out. They seem incorrect, since `malleated_valid` is created the exact same way as `malleated_invalid` below. Uncommenting this results in the test failing because `sendrawtransaction` fails with `mandartory-script-verify-flag-failed` but is not caught.
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2051507464)
I don't understand why these lines are commented out. They seem incorrect, since `malleated_valid` is created the exact same way as `malleated_invalid` below. Uncommenting this results in the test failing because `sendrawtransaction` fails with `mandartory-script-verify-flag-failed` but is not caught.
💬 andrewtoth commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2051515684)
I think I understand, the line `How to create a malleated valid witness?` is wondering if there is a way. Then instead of `malleate_tx` we can do the valid way instead.
Maybe that can be made more clear with a comment like: "TODO: Create a malleated valid witness (how?) and substitute `malleate_tx` below?"
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2051515684)
I think I understand, the line `How to create a malleated valid witness?` is wondering if there is a way. Then instead of `malleate_tx` we can do the valid way instead.
Maybe that can be made more clear with a comment like: "TODO: Create a malleated valid witness (how?) and substitute `malleate_tx` below?"
💬 andrewtoth commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-2816767319)
ACK 2b34857ad54aebbf0a9271e742c2caee85577bc8 - modulo the functional test malleated valid witness comment
Made many successful private broadcast connections with all 4 network types.
Successfully had connections timeout after sending the INV.
Tested resending the same raw tx multiple times.
Tested sending many double spends of the same tx at the same time.
Successfully had stale txs reattempted.
Tested with both `-maxconnections=0` and many inbound and outbound connections.
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-2816767319)
ACK 2b34857ad54aebbf0a9271e742c2caee85577bc8 - modulo the functional test malleated valid witness comment
Made many successful private broadcast connections with all 4 network types.
Successfully had connections timeout after sending the INV.
Tested resending the same raw tx multiple times.
Tested sending many double spends of the same tx at the same time.
Successfully had stale txs reattempted.
Tested with both `-maxconnections=0` and many inbound and outbound connections.
🤔 i-am-yuvi reviewed a pull request: "test: Run all benchmarks in the sanity check"
(https://github.com/bitcoin/bitcoin/pull/32310#pullrequestreview-2780062597)
Concept ACK
Why is the CI failing?
(https://github.com/bitcoin/bitcoin/pull/32310#pullrequestreview-2780062597)
Concept ACK
Why is the CI failing?
💬 l0rinc commented on pull request "[IBD] flush UTXOs in bigger batches based on dbcache size":
(https://github.com/bitcoin/bitcoin/pull/31645#discussion_r2051557904)
Updated it and rebased to fix build - please re-review.
(https://github.com/bitcoin/bitcoin/pull/31645#discussion_r2051557904)
Updated it and rebased to fix build - please re-review.
💬 l0rinc commented on pull request "test: Run all benchmarks in the sanity check":
(https://github.com/bitcoin/bitcoin/pull/32310#issuecomment-2816824275)
Concept ACK, depends on https://github.com/bitcoin/bitcoin/pull/32309 to make the build pass
(https://github.com/bitcoin/bitcoin/pull/32310#issuecomment-2816824275)
Concept ACK, depends on https://github.com/bitcoin/bitcoin/pull/32309 to make the build pass
💬 venpisey commented on pull request "Make TxGraph fuzz tests more deterministic":
(https://github.com/bitcoin/bitcoin/pull/32191#issuecomment-2816825465)
5167170015919920
(https://github.com/bitcoin/bitcoin/pull/32191#issuecomment-2816825465)
5167170015919920
💬 jonatack commented on pull request "rpc: add cpu_load to getpeerinfo":
(https://github.com/bitcoin/bitcoin/pull/31672#issuecomment-2816826222)
Latest changes since my last review LGTM. I plan to review the code more closely.
Empirically, running a node on this branch, it looks like over time most peers settle into less than 0.5 per milles, a few are higher, with maybe one peer around 4. These higher-load connections seem to more often be outbound ones.
(Edit, updated the branch with the -netinfo commit with a few improvements and to also only display cpu load for a peer (rounded to nearest integer) if it is non-zero (e.g. >= 0.5
...
(https://github.com/bitcoin/bitcoin/pull/31672#issuecomment-2816826222)
Latest changes since my last review LGTM. I plan to review the code more closely.
Empirically, running a node on this branch, it looks like over time most peers settle into less than 0.5 per milles, a few are higher, with maybe one peer around 4. These higher-load connections seem to more often be outbound ones.
(Edit, updated the branch with the -netinfo commit with a few improvements and to also only display cpu load for a peer (rounded to nearest integer) if it is non-zero (e.g. >= 0.5
...
💬 l0rinc commented on pull request "[IBD] prevector: allocate `P2WSH`/`P2TR`/`P2PK` scripts on stack":
(https://github.com/bitcoin/bitcoin/pull/32279#discussion_r2051562712)
I could, of course, for now I was going for simplifying the diff, not make the code more readable. I can of course do that later, but for now I'm looking for the simplest diff which demonstrates the problem and the proposed solution, and after I'm sure there's general support for it, we can sneak in code refactors as well.
(https://github.com/bitcoin/bitcoin/pull/32279#discussion_r2051562712)
I could, of course, for now I was going for simplifying the diff, not make the code more readable. I can of course do that later, but for now I'm looking for the simplest diff which demonstrates the problem and the proposed solution, and after I'm sure there's general support for it, we can sneak in code refactors as well.
💬 sipa commented on pull request "[IBD] prevector: allocate `P2WSH`/`P2TR`/`P2PK` scripts on stack":
(https://github.com/bitcoin/bitcoin/pull/32279#issuecomment-2816837610)
Nit: in title/description of the PR, I don't think it's accurate to say "on the stack". While there are occasionally CScript objects created on the stack, the vast majority is inside CCoinsCacheDb, where they are still heap-allocated. The benefit of the change here is that some of these would no longer need a *separate* heap allocation per CScript.
(https://github.com/bitcoin/bitcoin/pull/32279#issuecomment-2816837610)
Nit: in title/description of the PR, I don't think it's accurate to say "on the stack". While there are occasionally CScript objects created on the stack, the vast majority is inside CCoinsCacheDb, where they are still heap-allocated. The benefit of the change here is that some of these would no longer need a *separate* heap allocation per CScript.
💬 l0rinc commented on pull request "[IBD] prevector: store `P2WSH`/`P2TR`/`P2PK` scripts inline":
(https://github.com/bitcoin/bitcoin/pull/32279#issuecomment-2816840182)
Thank you, I have used inline/stack interchangeably - this is a lot better, appreciate the correction.
(https://github.com/bitcoin/bitcoin/pull/32279#issuecomment-2816840182)
Thank you, I have used inline/stack interchangeably - this is a lot better, appreciate the correction.
💬 l0rinc commented on pull request "bench: close wallets after migration":
(https://github.com/bitcoin/bitcoin/pull/32309#discussion_r2051568605)
Pushed again, please recheck
(https://github.com/bitcoin/bitcoin/pull/32309#discussion_r2051568605)
Pushed again, please recheck
🤔 mzumsande reviewed a pull request: "bench: close wallets after migration"
(https://github.com/bitcoin/bitcoin/pull/32309#pullrequestreview-2780087926)
Would be good to explain 1.) Why it failed before and 2.) How this is fixed
Looking at the commits, it seems that there are two things going on that seem contradictory:
Making sure that the benchmarks could have multiple invocations (commit 1 and 3), and making sure that it actually only runs once (commit 2).
(https://github.com/bitcoin/bitcoin/pull/32309#pullrequestreview-2780087926)
Would be good to explain 1.) Why it failed before and 2.) How this is fixed
Looking at the commits, it seems that there are two things going on that seem contradictory:
Making sure that the benchmarks could have multiple invocations (commit 1 and 3), and making sure that it actually only runs once (commit 2).
💬 l0rinc commented on pull request "bench: close wallets after migration":
(https://github.com/bitcoin/bitcoin/pull/32309#issuecomment-2816868935)
Thanks, I have updated the description, let me know if it answered your questions.
Short answer anyway: we have to clean up regardless of the execution count - and the benchmark seems to have been designed for a single execution anyway, otherwise recreating the wallet might have been too costly.
Let me know if you have a better idea.
(https://github.com/bitcoin/bitcoin/pull/32309#issuecomment-2816868935)
Thanks, I have updated the description, let me know if it answered your questions.
Short answer anyway: we have to clean up regardless of the execution count - and the benchmark seems to have been designed for a single execution anyway, otherwise recreating the wallet might have been too costly.
Let me know if you have a better idea.
💬 fjahr commented on issue "Mistake in `feature_pruning.py` functional test?":
(https://github.com/bitcoin/bitcoin/issues/32249#issuecomment-2816873696)
> I’ll investigate this further and determine the best way to implement a fix.
@EniRox001 I think we spoke on IRC but you went offline and I couldn't send you this last message. I took a brief look at the failure and it seems to me that the problem is that node 2 is being used in the reorg test previously. That's why it's on a different chain than the other nodes and can't fetch the last pruned block from the other nodes because these other nodes don't know this block. Fixing the typo surfaces
...
(https://github.com/bitcoin/bitcoin/issues/32249#issuecomment-2816873696)
> I’ll investigate this further and determine the best way to implement a fix.
@EniRox001 I think we spoke on IRC but you went offline and I couldn't send you this last message. I took a brief look at the failure and it seems to me that the problem is that node 2 is being used in the reorg test previously. That's why it's on a different chain than the other nodes and can't fetch the last pruned block from the other nodes because these other nodes don't know this block. Fixing the typo surfaces
...