💬 furszy commented on pull request "Disable util::Result copying and assignment":
(https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1579500335)
Not for this PR but.. we could remove this `Update` call pretty easily (e8a330584571bf0270c692676a57ebcf7d7c7697). We are calling `getNewDestination` twice only to verify if the wallet is locked or not.
(https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1579500335)
Not for this PR but.. we could remove this `Update` call pretty easily (e8a330584571bf0270c692676a57ebcf7d7c7697). We are calling `getNewDestination` twice only to verify if the wallet is locked or not.
💬 furszy commented on pull request "Disable util::Result copying and assignment":
(https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1579447042)
q: why uppercase? Do you wanted to differentiate own functions vs standard ones?
(https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1579447042)
q: why uppercase? Do you wanted to differentiate own functions vs standard ones?
👍 willcl-ark approved a pull request: "test: Add test case for spending bare multisig"
(https://github.com/bitcoin/bitcoin/pull/29120#pullrequestreview-2022689116)
ACK 9c7b2d0d37b31f01403f7a0f2ea25b60b126c841
Confirmed this tests that when the `-permitbaremultisig=0` policy is set, the mempool still permits spending (confirmed) bare multisig outputs. Mempool rejection of this output type is already tested earlier in the same test file.
(https://github.com/bitcoin/bitcoin/pull/29120#pullrequestreview-2022689116)
ACK 9c7b2d0d37b31f01403f7a0f2ea25b60b126c841
Confirmed this tests that when the `-permitbaremultisig=0` policy is set, the mempool still permits spending (confirmed) bare multisig outputs. Mempool rejection of this output type is already tested earlier in the same test file.
👍 willcl-ark approved a pull request: "contrib: rpcauth.py - Add new option (-json) to output text in json format"
(https://github.com/bitcoin/bitcoin/pull/29433#pullrequestreview-2022714587)
tACK 9adf949d2aa6d199b85295b18c08967395b5570a
Makes sense to me to be able to output this in JSON for automated setups.
(https://github.com/bitcoin/bitcoin/pull/29433#pullrequestreview-2022714587)
tACK 9adf949d2aa6d199b85295b18c08967395b5570a
Makes sense to me to be able to output this in JSON for automated setups.
👍 ryanofsky approved a pull request: "[refactor] Check CTxMemPool options in ctor"
(https://github.com/bitcoin/bitcoin/pull/28830#pullrequestreview-2022700917)
Code review ACK 30a1024b63105a97d04149e83ae2d8cf0830cf69
> Wasn't sure what `std::error& error` referred to, so just chose `util::Error`, but would be easy enough to use something else.
Sorry, that was just a confusing typo. I mean to write `std::string& error`. But I guess that doesn't work because the error message is actually translated. In that case, I do think `bilingual& error` would be better than `util::Error& error` just to make code simpler and more obvious. But no strong opinion
...
(https://github.com/bitcoin/bitcoin/pull/28830#pullrequestreview-2022700917)
Code review ACK 30a1024b63105a97d04149e83ae2d8cf0830cf69
> Wasn't sure what `std::error& error` referred to, so just chose `util::Error`, but would be easy enough to use something else.
Sorry, that was just a confusing typo. I mean to write `std::string& error`. But I guess that doesn't work because the error message is actually translated. In that case, I do think `bilingual& error` would be better than `util::Error& error` just to make code simpler and more obvious. But no strong opinion
...
💬 ryanofsky commented on pull request "[refactor] Check CTxMemPool options in ctor":
(https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1579587621)
In commit "[[refactor]] Check CTxMemPool options in constructor" (30a1024b63105a97d04149e83ae2d8cf0830cf69)
Would be good to add comment pointing out that error is currently ignored. (And maybe that this ensures mempool code passes fuzz tests when even max mempool size is unreasonably small.)
(https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1579587621)
In commit "[[refactor]] Check CTxMemPool options in constructor" (30a1024b63105a97d04149e83ae2d8cf0830cf69)
Would be good to add comment pointing out that error is currently ignored. (And maybe that this ensures mempool code passes fuzz tests when even max mempool size is unreasonably small.)
💬 npslaney commented on issue "Improving fee estimation accuracy":
(https://github.com/bitcoin/bitcoin/issues/27995#issuecomment-2077440875)
Bitcoin's fee estimator is outdated for sure. For lightning channels, where some functionality depends on accurate fee estimation, fee spikes like what we had last week can make small channels that are typically usable unusable, and that effect lasts longer due to bitcoind's fee estimation lagging what it actually takes to get into the next block.
Lightning peers have to agree on fees to an extent to function. We would love to pipe in our own fee estimation, but we are stuck in a lowest commo
...
(https://github.com/bitcoin/bitcoin/issues/27995#issuecomment-2077440875)
Bitcoin's fee estimator is outdated for sure. For lightning channels, where some functionality depends on accurate fee estimation, fee spikes like what we had last week can make small channels that are typically usable unusable, and that effect lasts longer due to bitcoind's fee estimation lagging what it actually takes to get into the next block.
Lightning peers have to agree on fees to an extent to function. We would love to pipe in our own fee estimation, but we are stuck in a lowest commo
...
💬 Sjors commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-2077460938)
Sounds like a bug on our end then. The BIP says:
> outpoint (36 bytes): the `COutPoint` of an input (32-byte txid, least significant byte first || 4-byte vout, least significant byte first)
And there's a footnote saying "Why are outpoints little-endian".
https://github.com/bitcoin/bips/blob/57c89ae162b4dab971dc6061ba6acf7d676781ea/bip-0352.mediawiki#cite_ref-why_little_endian_8-0
We should probably have a BIP352 specific `COutPoint` sorter, and a test vector.
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-2077460938)
Sounds like a bug on our end then. The BIP says:
> outpoint (36 bytes): the `COutPoint` of an input (32-byte txid, least significant byte first || 4-byte vout, least significant byte first)
And there's a footnote saying "Why are outpoints little-endian".
https://github.com/bitcoin/bips/blob/57c89ae162b4dab971dc6061ba6acf7d676781ea/bip-0352.mediawiki#cite_ref-why_little_endian_8-0
We should probably have a BIP352 specific `COutPoint` sorter, and a test vector.
👋 fanquake's pull request is ready for review: "depends: swap some cctools tools for LLVM tools"
(https://github.com/bitcoin/bitcoin/pull/29739)
(https://github.com/bitcoin/bitcoin/pull/29739)
💬 ryanofsky commented on pull request "Disable util::Result copying and assignment":
(https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1579633610)
re: https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1579447042
Personally I think using lowercase for instance methods and uppercase for other methods and functions is good practice because it makes it obvious when you are passing an implicit `this` parameter to function calls. (It is useful for the same reason `m_` prefixes are useful.) However, I got pushback when I tried to allow this in the style guide in #14635 so now I usually try to follow the guide.
(https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1579633610)
re: https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1579447042
Personally I think using lowercase for instance methods and uppercase for other methods and functions is good practice because it makes it obvious when you are passing an implicit `this` parameter to function calls. (It is useful for the same reason `m_` prefixes are useful.) However, I got pushback when I tried to allow this in the style guide in #14635 so now I usually try to follow the guide.
💬 ryanofsky commented on pull request "Disable util::Result copying and assignment":
(https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1579632793)
re: https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1579500335
It seems like the goal of the code is to get a new address without unlocking. But I haven't looked into this. It definitely looks a little clumsy, though, because it isn't actually using the error message that is returned.
(https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1579632793)
re: https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1579500335
It seems like the goal of the code is to get a new address without unlocking. But I haven't looked into this. It definitely looks a little clumsy, though, because it isn't actually using the error message that is returned.
💬 setavenger commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-2077491415)
> We should probably have a BIP352 specific `COutPoint` sorter, and a test vector.
Yeah, I'm working on a test vector that incorporates this kind of scenario.
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-2077491415)
> We should probably have a BIP352 specific `COutPoint` sorter, and a test vector.
Yeah, I'm working on a test vector that incorporates this kind of scenario.
💬 fanquake commented on pull request "depends: build miniupnpc with CMake":
(https://github.com/bitcoin/bitcoin/pull/29707#issuecomment-2077505030)
> FWIW, I found another Windows-specific bug in the upstream: https://github.com/miniupnp/miniupnp/pull/727.
Rebased again, but can pull in this change + fixups on next push
(https://github.com/bitcoin/bitcoin/pull/29707#issuecomment-2077505030)
> FWIW, I found another Windows-specific bug in the upstream: https://github.com/miniupnp/miniupnp/pull/727.
Rebased again, but can pull in this change + fixups on next push
💬 fjahr commented on pull request "contrib: Add asmap-tool":
(https://github.com/bitcoin/bitcoin/pull/28793#discussion_r1579698526)
done
(https://github.com/bitcoin/bitcoin/pull/28793#discussion_r1579698526)
done
💬 fjahr commented on pull request "contrib: Add asmap-tool":
(https://github.com/bitcoin/bitcoin/pull/28793#discussion_r1579698620)
done
(https://github.com/bitcoin/bitcoin/pull/28793#discussion_r1579698620)
done
💬 fjahr commented on pull request "contrib: Add asmap-tool":
(https://github.com/bitcoin/bitcoin/pull/28793#discussion_r1579699392)
Thanks for this hint but I think usage of `pathlib` is usually preferred now because it is more modern. So I will keep this as is unless there are more pros to this change that I have overlooked.
(https://github.com/bitcoin/bitcoin/pull/28793#discussion_r1579699392)
Thanks for this hint but I think usage of `pathlib` is usually preferred now because it is more modern. So I will keep this as is unless there are more pros to this change that I have overlooked.
💬 fjahr commented on pull request "contrib: Add asmap-tool":
(https://github.com/bitcoin/bitcoin/pull/28793#issuecomment-2077562279)
Addressed comments from @virtu , thanks for reviewing!
(https://github.com/bitcoin/bitcoin/pull/28793#issuecomment-2077562279)
Addressed comments from @virtu , thanks for reviewing!
💬 sr-gi commented on pull request "net: Decrease nMaxIPs when learning from DNS seeds":
(https://github.com/bitcoin/bitcoin/pull/29850#issuecomment-2077565724)
Post-merge ACK f2e3662e57eca1330962faf38ff428a564d50a11
(https://github.com/bitcoin/bitcoin/pull/29850#issuecomment-2077565724)
Post-merge ACK f2e3662e57eca1330962faf38ff428a564d50a11
💬 twosatsmaxi commented on pull request "policy: Allow non-standard scripts with -acceptnonstdtxn=1 (test nets only)":
(https://github.com/bitcoin/bitcoin/pull/29843#issuecomment-2077629808)
Following.
(https://github.com/bitcoin/bitcoin/pull/29843#issuecomment-2077629808)
Following.
📝 theStack opened a pull request: "refactor: remove remaining unused code from cpp-subprocess"
(https://github.com/bitcoin/bitcoin/pull/29961)
This PR removes remaining code that is unused within the cpp-subprocess module (templates and commented out code). Happy to add more removals if anyone finds more unused parts. Note that there are some API functions of the `Popen` class that we don't use, e.g. `wait()`, `pid()`, `poll()`, `kill()`, but they sound IMHO common enough to be useful in the future, so not sure how deep we should go there.
(https://github.com/bitcoin/bitcoin/pull/29961)
This PR removes remaining code that is unused within the cpp-subprocess module (templates and commented out code). Happy to add more removals if anyone finds more unused parts. Note that there are some API functions of the `Popen` class that we don't use, e.g. `wait()`, `pid()`, `poll()`, `kill()`, but they sound IMHO common enough to be useful in the future, so not sure how deep we should go there.