💬 Sun0fABeach commented on pull request "doc: revert clarify -datacarriersize":
(https://github.com/bitcoin/bitcoin/pull/29173#issuecomment-1876832867)
ACK. Good job catching a developer trying to sweep an important issue under the rug. Very concerning tbh.
(https://github.com/bitcoin/bitcoin/pull/29173#issuecomment-1876832867)
ACK. Good job catching a developer trying to sweep an important issue under the rug. Very concerning tbh.
✅ glozow closed a pull request: "doc: revert clarify -datacarriersize"
(https://github.com/bitcoin/bitcoin/pull/29173)
(https://github.com/bitcoin/bitcoin/pull/29173)
💬 glozow commented on pull request "doc: revert clarify -datacarriersize":
(https://github.com/bitcoin/bitcoin/pull/29173#issuecomment-1876847952)
NACK, the current documentation is more accurate to what the code actually does.
If you want to change what the code does, that's already its own PR #28408.
(https://github.com/bitcoin/bitcoin/pull/29173#issuecomment-1876847952)
NACK, the current documentation is more accurate to what the code actually does.
If you want to change what the code does, that's already its own PR #28408.
📝 torkelrogstad opened a pull request: "rpc: validate fee estimation mode case insensitive"
(https://github.com/bitcoin/bitcoin/pull/29175)
The RPC docs for `send` claim that `estimate_mode` is case insensitive. This is not the case on master:
https://github.com/bitcoin/bitcoin/blob/65c05db660b2ca1d0076b0d8573a6760b3228068/src/wallet/rpc/spend.cpp#L193-L195
This PR fixes that, by converting the estimation mode to lower case before comparing to `"unset"`. Note that the actual _parsing_ of the estimation mode already happens case insensitively, further down in the same function.
(https://github.com/bitcoin/bitcoin/pull/29175)
The RPC docs for `send` claim that `estimate_mode` is case insensitive. This is not the case on master:
https://github.com/bitcoin/bitcoin/blob/65c05db660b2ca1d0076b0d8573a6760b3228068/src/wallet/rpc/spend.cpp#L193-L195
This PR fixes that, by converting the estimation mode to lower case before comparing to `"unset"`. Note that the actual _parsing_ of the estimation mode already happens case insensitively, further down in the same function.
👍 TheCharlatan approved a pull request: "fuzz: rule-out too deep derivation paths in descriptor parsing targets"
(https://github.com/bitcoin/bitcoin/pull/28832#pullrequestreview-1803919066)
ACK a44808fb437864878c2d9696b8a96193091446ee
(https://github.com/bitcoin/bitcoin/pull/28832#pullrequestreview-1803919066)
ACK a44808fb437864878c2d9696b8a96193091446ee
💬 Retropex commented on pull request "doc: revert clarify -datacarriersize":
(https://github.com/bitcoin/bitcoin/pull/29173#issuecomment-1876904596)
@glozow I see, the [status quo](https://x.com/achow101/status/1735310357386731533?s=20) only applies when it suits you...
It's 50/50 of (N)Ack, it's not what I call an uncontrovesial change...
You do well to mention this PR because strangely it was [not](https://x.com/achow101/status/1735310357386731533?s=20) merged because of this piece of text.
(https://github.com/bitcoin/bitcoin/pull/29173#issuecomment-1876904596)
@glozow I see, the [status quo](https://x.com/achow101/status/1735310357386731533?s=20) only applies when it suits you...
It's 50/50 of (N)Ack, it's not what I call an uncontrovesial change...
You do well to mention this PR because strangely it was [not](https://x.com/achow101/status/1735310357386731533?s=20) merged because of this piece of text.
💬 kristapsk commented on pull request "doc: revert clarify -datacarriersize":
(https://github.com/bitcoin/bitcoin/pull/29173#issuecomment-1876918909)
@Retropex She is right, if you want to revert doc, code should also be changed to reflect that.
(https://github.com/bitcoin/bitcoin/pull/29173#issuecomment-1876918909)
@Retropex She is right, if you want to revert doc, code should also be changed to reflect that.
💬 Retropex commented on pull request "doc: revert clarify -datacarriersize":
(https://github.com/bitcoin/bitcoin/pull/29173#issuecomment-1876927476)
The purpose of `-datacarriersize` has ALWAYS been, as its name suggests, to enable or deactivate the relay of data carrier transactions.
However, a bug to bypass this function was found on December 14, 2022.
Literally **no** developer in the world corrects flaws by changing the documentation.
(https://github.com/bitcoin/bitcoin/pull/29173#issuecomment-1876927476)
The purpose of `-datacarriersize` has ALWAYS been, as its name suggests, to enable or deactivate the relay of data carrier transactions.
However, a bug to bypass this function was found on December 14, 2022.
Literally **no** developer in the world corrects flaws by changing the documentation.
💬 glozow commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1876929590)
>> This is a mempool policy proposal; I see no precedent that such a change requires a BIP.
> BIP-125 Replace-By-Fee comes to mind... which was written just over 8 years
ago, when Lightning didn't even exist, and RBF's intended purpose was just to
let people fee bump their own on-chain transactions. It was a much simpler
time, and even then it got a BIP.
A BIP was not required for RBF to be considered or merged. PR #6871 was merged on Nov 27, 2015.
After [discussion about adding docum
...
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1876929590)
>> This is a mempool policy proposal; I see no precedent that such a change requires a BIP.
> BIP-125 Replace-By-Fee comes to mind... which was written just over 8 years
ago, when Lightning didn't even exist, and RBF's intended purpose was just to
let people fee bump their own on-chain transactions. It was a much simpler
time, and even then it got a BIP.
A BIP was not required for RBF to be considered or merged. PR #6871 was merged on Nov 27, 2015.
After [discussion about adding docum
...
📝 maflcko opened a pull request: "wallet: Fix use-after-free in WalletBatch::EraseRecords"
(https://github.com/bitcoin/bitcoin/pull/29176)
Creating a copy of the pointer to the underlying data of the stream is not enough to copy the data.
Currently this happens to work sometimes, because the stream may not immediately free unused memory. However, there is no guarantee by the stream interface to always behave this way. Also, if `vector::clear` is called on the underlying memory, any pointers to it are invalid.
Fix this, by creating a full copy of all bytes.
(https://github.com/bitcoin/bitcoin/pull/29176)
Creating a copy of the pointer to the underlying data of the stream is not enough to copy the data.
Currently this happens to work sometimes, because the stream may not immediately free unused memory. However, there is no guarantee by the stream interface to always behave this way. Also, if `vector::clear` is called on the underlying memory, any pointers to it are invalid.
Fix this, by creating a full copy of all bytes.
💬 maflcko commented on pull request "wallet: Fix use-after-free in WalletBatch::EraseRecords":
(https://github.com/bitcoin/bitcoin/pull/29176#issuecomment-1876939948)
I believe this bug existed since the code was written in commit 22401f17e026ead4bc3fe96967eec56a719a4f75.
If one wants to trigger the bug in any c++ standard library, forcing a clear to also shrink may work:
```diff
diff --git a/src/streams.h b/src/streams.h
index bc04a2babd..26320db602 100644
--- a/src/streams.h
+++ b/src/streams.h
@@ -10,6 +10,7 @@
#include <span.h>
#include <support/allocators/zeroafterfree.h>
#include <util/overflow.h>
+#include <util/vector.h>
#includ
...
(https://github.com/bitcoin/bitcoin/pull/29176#issuecomment-1876939948)
I believe this bug existed since the code was written in commit 22401f17e026ead4bc3fe96967eec56a719a4f75.
If one wants to trigger the bug in any c++ standard library, forcing a clear to also shrink may work:
```diff
diff --git a/src/streams.h b/src/streams.h
index bc04a2babd..26320db602 100644
--- a/src/streams.h
+++ b/src/streams.h
@@ -10,6 +10,7 @@
#include <span.h>
#include <support/allocators/zeroafterfree.h>
#include <util/overflow.h>
+#include <util/vector.h>
#includ
...
💬 fanquake commented on pull request "Update libsecp256k1 subtree for 0.4.1 release":
(https://github.com/bitcoin/bitcoin/pull/29169#issuecomment-1876945410)
> It makes a minor difference of reporting the correct version number.
What do you mean by/where are we reporting the version number?
(https://github.com/bitcoin/bitcoin/pull/29169#issuecomment-1876945410)
> It makes a minor difference of reporting the correct version number.
What do you mean by/where are we reporting the version number?
💬 crediblebytes commented on pull request "doc: revert clarify -datacarriersize":
(https://github.com/bitcoin/bitcoin/pull/29173#issuecomment-1876950847)
You never change software requirements to fit the functionality. You either revisit the requirements to "Build the right software" or make the fix to "Build the software right". There was no bug fix but a lazy man's attempt to hide the bug at best. Honestly this person would be reprimanded for such negligence. Put the textual description back to what it was and provide a fix. Ya'll puttin development to shame.
(https://github.com/bitcoin/bitcoin/pull/29173#issuecomment-1876950847)
You never change software requirements to fit the functionality. You either revisit the requirements to "Build the right software" or make the fix to "Build the software right". There was no bug fix but a lazy man's attempt to hide the bug at best. Honestly this person would be reprimanded for such negligence. Put the textual description back to what it was and provide a fix. Ya'll puttin development to shame.
📝 hebasto opened a pull request: "build: Fix check whether `-latomic` needed"
(https://github.com/bitcoin/bitcoin/pull/29177)
Clang >=15 still might need linking against `libatomic`.
We use `std::atomic<std::chrono::seconds>::compare_exchange_strong` in `net_processing.cpp`.
Addresses the https://github.com/bitcoin/bitcoin/pull/29165#discussion_r1440293694.
(https://github.com/bitcoin/bitcoin/pull/29177)
Clang >=15 still might need linking against `libatomic`.
We use `std::atomic<std::chrono::seconds>::compare_exchange_strong` in `net_processing.cpp`.
Addresses the https://github.com/bitcoin/bitcoin/pull/29165#discussion_r1440293694.
💬 fanquake commented on issue "Broken `--enable-suppress-external-warning` for Apple Clang 15 on `x86_64`":
(https://github.com/bitcoin/bitcoin/issues/29174#issuecomment-1876969998)
> fails with error: unknown argument: '-internal-isystem/usr/local/include'
Can this be investigated further. The `-Xclang` and `-internal-isystem` options still exist in Clang. Is the issue here just the concatenation? Maybe that worked previously, but no-longer does? What happens if you change `-Xclang -internal-isystem/usr/local/include` to `-Xclang -internal-isystem /usr/local/include`?
(https://github.com/bitcoin/bitcoin/issues/29174#issuecomment-1876969998)
> fails with error: unknown argument: '-internal-isystem/usr/local/include'
Can this be investigated further. The `-Xclang` and `-internal-isystem` options still exist in Clang. Is the issue here just the concatenation? Maybe that worked previously, but no-longer does? What happens if you change `-Xclang -internal-isystem/usr/local/include` to `-Xclang -internal-isystem /usr/local/include`?
💬 hebasto commented on pull request "build: Bump clang minimum supported version to 15":
(https://github.com/bitcoin/bitcoin/pull/29165#discussion_r1441658150)
Please see https://github.com/bitcoin/bitcoin/pull/29177.
(https://github.com/bitcoin/bitcoin/pull/29165#discussion_r1441658150)
Please see https://github.com/bitcoin/bitcoin/pull/29177.
💬 petertodd commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1876973577)
On Thu, Jan 04, 2024 at 01:06:19AM -0800, Bastien Teinturier wrote:
> > No, that is not how Lightning works. With anchor outputs, beyond the basic fee level, fees are paid by whichever side wants to pay them, potentially both parties at once.
>
> You're confusing fees paid directly by the commitment transaction (which are always paid by the channel initiator, regardless of whose commitment transaction it is) and fees paid by CPFP-ing the anchor output. Which doesn't make any sense to me since
...
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1876973577)
On Thu, Jan 04, 2024 at 01:06:19AM -0800, Bastien Teinturier wrote:
> > No, that is not how Lightning works. With anchor outputs, beyond the basic fee level, fees are paid by whichever side wants to pay them, potentially both parties at once.
>
> You're confusing fees paid directly by the commitment transaction (which are always paid by the channel initiator, regardless of whose commitment transaction it is) and fees paid by CPFP-ing the anchor output. Which doesn't make any sense to me since
...
💬 stickies-v commented on pull request "doc: revert clarify -datacarriersize":
(https://github.com/bitcoin/bitcoin/pull/29173#issuecomment-1876980521)
NACK. Rough consensus on behaviour change can be achieved on the other pull, we shouldn't have incorrect documentation while that process plays out.
(https://github.com/bitcoin/bitcoin/pull/29173#issuecomment-1876980521)
NACK. Rough consensus on behaviour change can be achieved on the other pull, we shouldn't have incorrect documentation while that process plays out.
💬 petertodd commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1876988120)
On Thu, Jan 04, 2024 at 03:19:50AM -0800, Gloria Zhao wrote:
> >> This is a mempool policy proposal; I see no precedent that such a change requires a BIP.
>
> > BIP-125 Replace-By-Fee comes to mind... which was written just over 8 years
> ago, when Lightning didn't even exist, and RBF's intended purpose was just to
> let people fee bump their own on-chain transactions. It was a much simpler
> time, and even then it got a BIP.
>
> A BIP was not required for RBF to be considered or merged. PR #
...
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1876988120)
On Thu, Jan 04, 2024 at 03:19:50AM -0800, Gloria Zhao wrote:
> >> This is a mempool policy proposal; I see no precedent that such a change requires a BIP.
>
> > BIP-125 Replace-By-Fee comes to mind... which was written just over 8 years
> ago, when Lightning didn't even exist, and RBF's intended purpose was just to
> let people fee bump their own on-chain transactions. It was a much simpler
> time, and even then it got a BIP.
>
> A BIP was not required for RBF to be considered or merged. PR #
...
⚠️ fanquake opened an issue: "ubsan: misaligned-pointer-use in crc32c/src/crc32c_arm64.cc"
(https://github.com/bitcoin/bitcoin/issues/29178)
master @ 65c05db660b2ca1d0076b0d8573a6760b3228068.
Running `time FILE_ENV="./ci/test/00_setup_env_native_fuzz.sh" ./ci/test_run_all.sh` on Rawhide aarch64.
```bash
Run coincontrol with args ['/ci_container_base/ci/scratch/build/bitcoin-aarch64-unknown-linux-gnu/src/test/fuzz/fuzz', '-runs=1', PosixPath('/ci_container_base/ci/scratch/qa-assets/fuzz_seed_corpus/coincontrol')]crc32c/src/crc32c_arm64.cc:101:26: runtime error: load of misaligned address 0x52d000000406 for type 'uint64_t' (aka 'u
...
(https://github.com/bitcoin/bitcoin/issues/29178)
master @ 65c05db660b2ca1d0076b0d8573a6760b3228068.
Running `time FILE_ENV="./ci/test/00_setup_env_native_fuzz.sh" ./ci/test_run_all.sh` on Rawhide aarch64.
```bash
Run coincontrol with args ['/ci_container_base/ci/scratch/build/bitcoin-aarch64-unknown-linux-gnu/src/test/fuzz/fuzz', '-runs=1', PosixPath('/ci_container_base/ci/scratch/qa-assets/fuzz_seed_corpus/coincontrol')]crc32c/src/crc32c_arm64.cc:101:26: runtime error: load of misaligned address 0x52d000000406 for type 'uint64_t' (aka 'u
...
💬 cculianu commented on pull request "datacarriersize: Match more datacarrying":
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1877009300)
> > Concept ACK
> > The transactions targeted by this pull-req. are a very significant source of prohibitive cost for regular node operators. It is very unlikely that regular node operators will give up mitigating that source of operative cost. Regular policy rule for these transactions would encourage the economical resource usage of mempools - not harming any miners - neither changing any fee estimation already on the field.
>
> As long as miners continue to mine these transactions they wi
...
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1877009300)
> > Concept ACK
> > The transactions targeted by this pull-req. are a very significant source of prohibitive cost for regular node operators. It is very unlikely that regular node operators will give up mitigating that source of operative cost. Regular policy rule for these transactions would encourage the economical resource usage of mempools - not harming any miners - neither changing any fee estimation already on the field.
>
> As long as miners continue to mine these transactions they wi
...