💬 instagibbs commented on pull request "wallet: Identify transactions spending 0-value outputs, and add tests for anchor outputs in a wallet":
(https://github.com/bitcoin/bitcoin/pull/33268#discussion_r2310110919)
this subtest is covering 0-value outputs, not anchors per se. I'd rather keep the tests logically separated to not confused the concepts more than they already are :)
(https://github.com/bitcoin/bitcoin/pull/33268#discussion_r2310110919)
this subtest is covering 0-value outputs, not anchors per se. I'd rather keep the tests logically separated to not confused the concepts more than they already are :)
💬 instagibbs commented on pull request "wallet: Identify transactions spending 0-value outputs, and add tests for anchor outputs in a wallet":
(https://github.com/bitcoin/bitcoin/pull/33268#issuecomment-3236975180)
@jsarenik technically this issue could happen if miners mine 0-value outputs of any kind, but given that it's now relay-possible, seems like a good idea to backport
(https://github.com/bitcoin/bitcoin/pull/33268#issuecomment-3236975180)
@jsarenik technically this issue could happen if miners mine 0-value outputs of any kind, but given that it's now relay-possible, seems like a good idea to backport
💬 ajtowns commented on pull request "net: Provide block templates to peers on request":
(https://github.com/bitcoin/bitcoin/pull/33191#issuecomment-3236980424)
Rebased over #33253 but not optimised to iterate over template txs efficiently.
(https://github.com/bitcoin/bitcoin/pull/33191#issuecomment-3236980424)
Rebased over #33253 but not optimised to iterate over template txs efficiently.
🤔 stickies-v reviewed a pull request: "rpc: refactor: use string_view in Arg/MaybeArg"
(https://github.com/bitcoin/bitcoin/pull/32983#pullrequestreview-3168699729)
Force-pushed to address review comments from @maflcko:
- fixed docstring
- removed usage of std::string_view::data() to avoid null termination issues in the future
- added one more usage of Arg<string_view> helper
(https://github.com/bitcoin/bitcoin/pull/32983#pullrequestreview-3168699729)
Force-pushed to address review comments from @maflcko:
- fixed docstring
- removed usage of std::string_view::data() to avoid null termination issues in the future
- added one more usage of Arg<string_view> helper
💬 stickies-v commented on pull request "rpc: refactor: use string_view in Arg/MaybeArg":
(https://github.com/bitcoin/bitcoin/pull/32983#discussion_r2310136874)
DELETE
(https://github.com/bitcoin/bitcoin/pull/32983#discussion_r2310136874)
DELETE
💬 stickies-v commented on pull request "rpc: refactor: use string_view in Arg/MaybeArg":
(https://github.com/bitcoin/bitcoin/pull/32983#discussion_r2310142429)
Done!
(https://github.com/bitcoin/bitcoin/pull/32983#discussion_r2310142429)
Done!
💬 stickies-v commented on pull request "rpc: refactor: use string_view in Arg/MaybeArg":
(https://github.com/bitcoin/bitcoin/pull/32983#discussion_r2310142118)
I agree. Even though there are no issues now, it's safer to avoid using `.data()` altogether. I've adopted your suggestion. I'll probably open a separate PR later today to change the necessary `net` code to allow us to `string_view` here. It's a relatively straightforward change, but I don't want to scope creep this PR more.
(https://github.com/bitcoin/bitcoin/pull/32983#discussion_r2310142118)
I agree. Even though there are no issues now, it's safer to avoid using `.data()` altogether. I've adopted your suggestion. I'll probably open a separate PR later today to change the necessary `net` code to allow us to `string_view` here. It's a relatively straightforward change, but I don't want to scope creep this PR more.
💬 stickies-v commented on pull request "rpc: refactor: use string_view in Arg/MaybeArg":
(https://github.com/bitcoin/bitcoin/pull/32983#discussion_r2310144112)
Done. Kept the `get_str()` because we need a `std::string`, but your suggestion is clean.
(https://github.com/bitcoin/bitcoin/pull/32983#discussion_r2310144112)
Done. Kept the `get_str()` because we need a `std::string`, but your suggestion is clean.
💬 bigshiny90 commented on pull request "test: Add functional tests for blockreconstructionextratxn and extra pool (compactblocks)":
(https://github.com/bitcoin/bitcoin/pull/33023#issuecomment-3237047651)
investigating why tests fail on github CI... but run fine on a local build
(https://github.com/bitcoin/bitcoin/pull/33023#issuecomment-3237047651)
investigating why tests fail on github CI... but run fine on a local build
🤔 janb84 reviewed a pull request: "test: p2p block malleability"
(https://github.com/bitcoin/bitcoin/pull/33172#pullrequestreview-3168806620)
re ACK d0e1bbad016cc4949094daea2934712f92dfeecd
Changes since last ACK;
- small NIT suggested changes
(https://github.com/bitcoin/bitcoin/pull/33172#pullrequestreview-3168806620)
re ACK d0e1bbad016cc4949094daea2934712f92dfeecd
Changes since last ACK;
- small NIT suggested changes
💬 bigshiny90 commented on pull request "test: Add functional tests for blockreconstructionextratxn and extra pool (compactblocks)":
(https://github.com/bitcoin/bitcoin/pull/33023#issuecomment-3237148592)
needed to rebase to current master and fix tests for the new lower fee-rate policy.
(https://github.com/bitcoin/bitcoin/pull/33023#issuecomment-3237148592)
needed to rebase to current master and fix tests for the new lower fee-rate policy.
📝 fanquake opened a pull request: "[29.x] rc3 or final"
(https://github.com/bitcoin/bitcoin/pull/33271)
Backports:
* #33236
Might include #33268.
Since `rc2` #33212 was also backported in #33251.
(https://github.com/bitcoin/bitcoin/pull/33271)
Backports:
* #33236
Might include #33268.
Since `rc2` #33212 was also backported in #33251.
💬 brunoerg commented on pull request "fuzz: enhance wallet_fees by mocking mempool stuff":
(https://github.com/bitcoin/bitcoin/pull/33210#discussion_r2310498917)
Done
(https://github.com/bitcoin/bitcoin/pull/33210#discussion_r2310498917)
Done
💬 brunoerg commented on pull request "fuzz: enhance wallet_fees by mocking mempool stuff":
(https://github.com/bitcoin/bitcoin/pull/33210#discussion_r2310501279)
I moved `MockMempoolMinFee` to mempool utils to use it here and in the unit tests. I had to add one more parameter which is the mempool to be mocked.
(https://github.com/bitcoin/bitcoin/pull/33210#discussion_r2310501279)
I moved `MockMempoolMinFee` to mempool utils to use it here and in the unit tests. I had to add one more parameter which is the mempool to be mocked.
💬 brunoerg commented on pull request "fuzz: enhance wallet_fees by mocking mempool stuff":
(https://github.com/bitcoin/bitcoin/pull/33210#discussion_r2310501512)
Done.
(https://github.com/bitcoin/bitcoin/pull/33210#discussion_r2310501512)
Done.
💬 brunoerg commented on pull request "fuzz: enhance wallet_fees by mocking mempool stuff":
(https://github.com/bitcoin/bitcoin/pull/33210#discussion_r2310502672)
Make sense, will leave it for a possible follow-up.
(https://github.com/bitcoin/bitcoin/pull/33210#discussion_r2310502672)
Make sense, will leave it for a possible follow-up.
💬 brunoerg commented on pull request "fuzz: enhance wallet_fees by mocking mempool stuff":
(https://github.com/bitcoin/bitcoin/pull/33210#discussion_r2310503817)
I think so, I can change it if I have to touch it again or in a follow-up.
(https://github.com/bitcoin/bitcoin/pull/33210#discussion_r2310503817)
I think so, I can change it if I have to touch it again or in a follow-up.
💬 0xB10C commented on issue "signet: disk-space-DoS due to low mining difficulty":
(https://github.com/bitcoin/bitcoin/issues/33266#issuecomment-3237765557)
> Would such blocks mined by anyone even be considered valid on signet since they would not contain any expected valid signature?
Yes. Just the nonce in the block header changes. The signet commitment commits to the header version, prevhash, merkleroot, and timestamp. It does not commit (and can not commit) to the nonce. The underlying block does not change. The commitment remains valid.
(https://github.com/bitcoin/bitcoin/issues/33266#issuecomment-3237765557)
> Would such blocks mined by anyone even be considered valid on signet since they would not contain any expected valid signature?
Yes. Just the nonce in the block header changes. The signet commitment commits to the header version, prevhash, merkleroot, and timestamp. It does not commit (and can not commit) to the nonce. The underlying block does not change. The commitment remains valid.
💬 achow101 commented on pull request "doc: unify `datacarriersize` warning with release notes":
(https://github.com/bitcoin/bitcoin/pull/33224#issuecomment-3237941668)
> Is this context info useful? Line numbers will change whenever some previous function in the file gets a non-trivial edit... Would it be better to add `-locations none` to the LCONVERT invocation in translate.cmake?
I don't think line numbers is the problem. The actual source location of the string can change and the ids in the `.xlf` won't change. It's the sort order in the `.ts` file that `lupdate` generates that seems to cause the id change. It's unclear to me why the sort order would ch
...
(https://github.com/bitcoin/bitcoin/pull/33224#issuecomment-3237941668)
> Is this context info useful? Line numbers will change whenever some previous function in the file gets a non-trivial edit... Would it be better to add `-locations none` to the LCONVERT invocation in translate.cmake?
I don't think line numbers is the problem. The actual source location of the string can change and the ids in the `.xlf` won't change. It's the sort order in the `.ts` file that `lupdate` generates that seems to cause the id change. It's unclear to me why the sort order would ch
...
👍 l0rinc approved a pull request: "test: Fixup fill_mempool docstring"
(https://github.com/bitcoin/bitcoin/pull/33269#pullrequestreview-3169923720)
ACK fa3f682032a3292604f363a5ee4557937f3d8950
(https://github.com/bitcoin/bitcoin/pull/33269#pullrequestreview-3169923720)
ACK fa3f682032a3292604f363a5ee4557937f3d8950