🤔 furszy reviewed a pull request: "test: ensure output is large enough to pay for its fees"
(https://github.com/bitcoin/bitcoin/pull/29283#pullrequestreview-1843832314)
utACK 3bfc5bd36
(https://github.com/bitcoin/bitcoin/pull/29283#pullrequestreview-1843832314)
utACK 3bfc5bd36
💬 furszy commented on pull request "test: ensure output is large enough to pay for its fees":
(https://github.com/bitcoin/bitcoin/pull/29283#discussion_r1466431816)
Maybe:
```suggestion
# Ensure output is large enough to pay for its fees. Following transactions will contain one input and one unspendable output. Conservatively assuming...
```
Also, just as an extra note (no need to do it): could calculate the floor in a more dynamic manner if you want. The input type is known here, and the future tx is always fixed to be a 1 input, 1 bech32 output tx.
(https://github.com/bitcoin/bitcoin/pull/29283#discussion_r1466431816)
Maybe:
```suggestion
# Ensure output is large enough to pay for its fees. Following transactions will contain one input and one unspendable output. Conservatively assuming...
```
Also, just as an extra note (no need to do it): could calculate the floor in a more dynamic manner if you want. The input type is known here, and the future tx is always fixed to be a 1 input, 1 bech32 output tx.
💬 Sjors commented on pull request "CKey: add Serialize and Unserialize":
(https://github.com/bitcoin/bitcoin/pull/29295#issuecomment-1910300031)
Switched to the approach suggested by @vasild. Also supports uncompressed keys. Added test.
Keeping this draft pending #29307.
(https://github.com/bitcoin/bitcoin/pull/29295#issuecomment-1910300031)
Switched to the approach suggested by @vasild. Also supports uncompressed keys. Added test.
Keeping this draft pending #29307.
👋 Sjors's pull request is ready for review: "CKey: add Serialize and Unserialize"
(https://github.com/bitcoin/bitcoin/pull/29295)
(https://github.com/bitcoin/bitcoin/pull/29295)
💬 Sjors commented on pull request "CKey: add Serialize and Unserialize":
(https://github.com/bitcoin/bitcoin/pull/29295#issuecomment-1910305918)
Come to think of it, this PR only changes serialization, while #29307 deals with storage. So this is ready for review.
(https://github.com/bitcoin/bitcoin/pull/29295#issuecomment-1910305918)
Come to think of it, this PR only changes serialization, while #29307 deals with storage. So this is ready for review.
💬 fanquake commented on pull request "depends: Do not override `CFLAGS` when building SQLite with `DEBUG=1`":
(https://github.com/bitcoin/bitcoin/pull/29287#issuecomment-1910315057)
oss-fuzz PR: https://github.com/google/oss-fuzz/pull/11540
(https://github.com/bitcoin/bitcoin/pull/29287#issuecomment-1910315057)
oss-fuzz PR: https://github.com/google/oss-fuzz/pull/11540
💬 stickies-v commented on pull request "test: ensure output is large enough to pay for its fees":
(https://github.com/bitcoin/bitcoin/pull/29283#discussion_r1466450971)
Thx, will consider if I need to retouch!
(https://github.com/bitcoin/bitcoin/pull/29283#discussion_r1466450971)
Thx, will consider if I need to retouch!
🚀 fanquake merged a pull request: "ci: Update cache action"
(https://github.com/bitcoin/bitcoin/pull/29313)
(https://github.com/bitcoin/bitcoin/pull/29313)
💬 chrisguida commented on pull request "Add a `-permitbarepubkey` option":
(https://github.com/bitcoin/bitcoin/pull/29309#issuecomment-1910328432)
Concept ACK, it's great to give users the option of filtering out potentially abusive txs :+1:
(https://github.com/bitcoin/bitcoin/pull/29309#issuecomment-1910328432)
Concept ACK, it's great to give users the option of filtering out potentially abusive txs :+1:
💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks (and multi-user)":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1466462619)
Oh, this is only available in Noble? In that case I might just improve the code comment (including `sleep`) and add a note that can go away with Noble.
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1466462619)
Oh, this is only available in Noble? In that case I might just improve the code comment (including `sleep`) and add a note that can go away with Noble.
🤔 furszy reviewed a pull request: "wallet: guard against dangling to-be-reverted db transactions"
(https://github.com/bitcoin/bitcoin/pull/29253#pullrequestreview-1843953699)
> I'm trying to understand this, but confused about why executing "ROLLBACK TRANSACTION" would be expected to fail in sqlite. It seems like the fix in [e5217fe](https://github.com/bitcoin/bitcoin/commit/e5217fea1516120643f4a7a55a474648e21e2269) is handling failure to roll back by trying to close and reopen the database. But I don't understand why rolling back a transaction would fail to begin with, unless there was some serious error like disk corruption. Probably to make the this fix clearer an
...
(https://github.com/bitcoin/bitcoin/pull/29253#pullrequestreview-1843953699)
> I'm trying to understand this, but confused about why executing "ROLLBACK TRANSACTION" would be expected to fail in sqlite. It seems like the fix in [e5217fe](https://github.com/bitcoin/bitcoin/commit/e5217fea1516120643f4a7a55a474648e21e2269) is handling failure to roll back by trying to close and reopen the database. But I don't understand why rolling back a transaction would fail to begin with, unless there was some serious error like disk corruption. Probably to make the this fix clearer an
...
💬 instagibbs commented on pull request "policy: enable sibling eviction for v3 transactions":
(https://github.com/bitcoin/bitcoin/pull/29306#discussion_r1466473683)
```Suggestion
// we don't conflict inputs with it, consider evicting it under RBF rules. We rely on v3 rules
```
(https://github.com/bitcoin/bitcoin/pull/29306#discussion_r1466473683)
```Suggestion
// we don't conflict inputs with it, consider evicting it under RBF rules. We rely on v3 rules
```
💬 instagibbs commented on pull request "policy: enable sibling eviction for v3 transactions":
(https://github.com/bitcoin/bitcoin/pull/29306#discussion_r1466475345)
aside: In relaxed topos we just attempt the eviction of *all* of them
(https://github.com/bitcoin/bitcoin/pull/29306#discussion_r1466475345)
aside: In relaxed topos we just attempt the eviction of *all* of them
💬 instagibbs commented on pull request "policy: enable sibling eviction for v3 transactions":
(https://github.com/bitcoin/bitcoin/pull/29306#discussion_r1466484490)
this return value needs to be explained very well
(https://github.com/bitcoin/bitcoin/pull/29306#discussion_r1466484490)
this return value needs to be explained very well
🤔 instagibbs reviewed a pull request: "policy: enable sibling eviction for v3 transactions"
(https://github.com/bitcoin/bitcoin/pull/29306#pullrequestreview-1843901103)
very compact!
alternative log lines are a bit noisy, but understandable and I have no good suggestions that reduce that much
(https://github.com/bitcoin/bitcoin/pull/29306#pullrequestreview-1843901103)
very compact!
alternative log lines are a bit noisy, but understandable and I have no good suggestions that reduce that much
💬 instagibbs commented on pull request "policy: enable sibling eviction for v3 transactions":
(https://github.com/bitcoin/bitcoin/pull/29306#discussion_r1466538340)
needs test coverage
(https://github.com/bitcoin/bitcoin/pull/29306#discussion_r1466538340)
needs test coverage
💬 instagibbs commented on pull request "policy: enable sibling eviction for v3 transactions":
(https://github.com/bitcoin/bitcoin/pull/29306#discussion_r1466531988)
did you consider internalizing this logic inside `ApplyV3` directly to reduce conceptual sprawl
(https://github.com/bitcoin/bitcoin/pull/29306#discussion_r1466531988)
did you consider internalizing this logic inside `ApplyV3` directly to reduce conceptual sprawl
👍 fanquake approved a pull request: "depends: Do not override `CFLAGS` when building SQLite with `DEBUG=1`"
(https://github.com/bitcoin/bitcoin/pull/29287#pullrequestreview-1844036451)
ACK 5fb8f0f80fc41cc636da56864195244d8fd9116e - downstream is also green, so i'll fixup the PR there.
(https://github.com/bitcoin/bitcoin/pull/29287#pullrequestreview-1844036451)
ACK 5fb8f0f80fc41cc636da56864195244d8fd9116e - downstream is also green, so i'll fixup the PR there.
💬 fanquake commented on pull request "depends: Do not override `CFLAGS` when building SQLite with `DEBUG=1`":
(https://github.com/bitcoin/bitcoin/pull/29287#discussion_r1466555308)
This should really be done globally, but I can follow up, as I have another global change to make at the same time.
(https://github.com/bitcoin/bitcoin/pull/29287#discussion_r1466555308)
This should really be done globally, but I can follow up, as I have another global change to make at the same time.
💬 fanquake commented on pull request "depends: Do not override `CFLAGS` when building SQLite with `DEBUG=1`":
(https://github.com/bitcoin/bitcoin/pull/29287#discussion_r1466556573)
This comment could be a bit confusing, because it says we don't want to use `--enable-debug`, because it overrides cflags, but then we override cflags in the next line. Not a blocker though.
(https://github.com/bitcoin/bitcoin/pull/29287#discussion_r1466556573)
This comment could be a bit confusing, because it says we don't want to use `--enable-debug`, because it overrides cflags, but then we override cflags in the next line. Not a blocker though.